davidker / unisys

Master repository for new changes to drivers/staging/unisys and drivers/visorbus
Other
2 stars 1 forks source link

Remove useless check #103

Open ghost opened 7 years ago

ghost commented 7 years ago
 >  } else {
 >      void *mapping = memremap(addr, bytes, MEMREMAP_WB);
 > 
 >      if (!mapping)
 >          goto err_finish_ctx;
 >      memcpy(ctx->data, mapping, bytes);
 >      memunmap(mapping);
 >  }
 > 
 >  ctx->byte_stream = true;
 >  controlvm_payload_bytes_buffered += ctx->param_bytes;
 > 
 >  return ctx;
 > 
 > err_finish_ctx:
 >  parser_done(ctx);
 >  return NULL;
 > }
 > 
 > static uuid_le
 > parser_id_get(struct parser_context *ctx)
 > {
 >  struct spar_controlvm_parameters_header *phdr = NULL;
 > 
 >  if (!ctx)
 >      return NULL_UUID_LE;

How can that ever be true? ...

 >  return phdr->id;
 > }
 > 
 > /*
 >  * Describes the state from the perspective of which controlvm messages have
 >  * been received for a bus or device.
 >  */
 > 
 > enum PARSER_WHICH_STRING {
 >  PARSERSTRING_INITIATOR,
 >  PARSERSTRING_TARGET,
 >  PARSERSTRING_CONNECTION,
 >  PARSERSTRING_NAME, /* TODO: only PARSERSTRING_NAME is used ? */
 > };
 > 
 > static void
 > parser_param_start(struct parser_context *ctx,
 >         enum PARSER_WHICH_STRING which_string)
 > {
 >  struct spar_controlvm_parameters_header *phdr = NULL;
 > 
 >  if (!ctx)
 >      return;

How could that happen?

...

 >  default:
 >      break;
 >  }
 > }
 > 
 > static void parser_done(struct parser_context *ctx)
 > {
 >  if (!ctx)
 >      return;

How can this happen?

...

 >  controlvm_payload_bytes_buffered -= ctx->param_bytes;
 >  kfree(ctx);
 > }
 > 
 > static void *
 > parser_string_get(struct parser_context *ctx)
 > {
 >  u8 *pscan;
 >  unsigned long nscan;
 >  int value_length = -1;
 >  void *value = NULL;
 >  int i;
 > 
 >  if (!ctx)
 >      return NULL;

Again, how?

ghost commented 7 years ago
$ grep -rn "if (\!.*)" *
visorbus/visorchannel.c:60: if (!channel)
visorbus/visorchannel.c:402:    if (!channel)
visorbus/visorchannel.c:417:    if (!channel->requested) {
visorbus/visorchannel.c:425:    if (!channel->mapped) {
visorbus/visorchannel.c:450:    if (!channel->requested) {
visorbus/visorchannel.c:459:    if (!channel->mapped) {

visorbus/visorbus_main.c:120:   if (!drv->channel_types)
visorbus/visorbus_main.c:188:   if (!vdev->visorchannel)
visorbus/visorbus_main.c:200:   if (!vdev->visorchannel)
visorbus/visorbus_main.c:212:   if (!vdev->visorchannel)
visorbus/visorbus_main.c:225:   if (!vdev->visorchannel)
visorbus/visorbus_main.c:238:   if (!vdev->visorchannel)
visorbus/visorbus_main.c:254:   if (!vdev->visorchannel || !xbus || !xdrv)
visorbus/visorbus_main.c:257:   if (!i)
visorbus/visorbus_main.c:391:   if (!channel)
visorbus/visorbus_main.c:461:   if (!dev->timer_active)
visorbus/visorbus_main.c:642:    *              if (!drv.probe(dev))  [visordriver_probe_device]
visorbus/visorbus_main.c:676:   if (!SPAR_VBUS_CHANNEL_OK_CLIENT(visorchannel_get_header(chan)))
visorbus/visorbus_main.c:793:   if (!visordev->device.driver)
visorbus/visorbus_main.c:797:   if (!bdev)
visorbus/visorbus_main.c:800:   if (!hdr_info)
visorbus/visorbus_main.c:856:   if (!drv->probe)
visorbus/visorbus_main.c:941:    *           if (!dev.drv)  ** no driver assigned yet **
visorbus/visorbus_main.c:944:    *               if (!drv.probe(dev))   [visordriver_probe_device]
visorbus/visorbus_main.c:972:   if (!hdr_info)
visorbus/visorbus_main.c:982:   if (!dev->debugfs_dir) {
visorbus/visorbus_main.c:990:   if (!dev->debugfs_client_bus_info) {
visorbus/visorbus_main.c:1157:  if (!dev->pausing)
visorbus/visorbus_main.c:1177:  if (!dev->resuming)
visorbus/visorbus_main.c:1212:  if (!notify_func)
visorbus/visorbus_main.c:1216:  if (!drv) {
visorbus/visorbus_main.c:1235:      if (!drv->pause) {
visorbus/visorbus_main.c:1250:      if (!drv->resume) {
visorbus/visorbus_main.c:1303:  if (!visorbus_debugfs_dir)

visorbus/visorchipset.c:356:    if (!ctx)
visorbus/visorchipset.c:380:    if (!ctx)
visorbus/visorchipset.c:408:    if (!ctx)
visorbus/visorchipset.c:423:    if (!ctx)
visorbus/visorchipset.c:429:    if (!pscan)
visorbus/visorchipset.c:439:    if (!value)
visorbus/visorchipset.c:643:    if (!pending_msg_hdr)
visorbus/visorchipset.c:661:    if (!p->pending_msg_hdr)
visorbus/visorchipset.c:682:    if (!pending_msg_hdr)
visorbus/visorchipset.c:698:    if (!bus_info) {                                    **// ???**
visorbus/visorchipset.c:716:        if (!pmsg_hdr) {
visorbus/visorchipset.c:751:    if (!dev_info) {                                    **// ???**
visorbus/visorchipset.c:769:        if (!pmsg_hdr) {
visorbus/visorchipset.c:829:    if (!bus_info) {
visorbus/visorchipset.c:847:    if (!visorchannel) {
visorbus/visorchipset.c:875:    if (!bus_info)
visorbus/visorchipset.c:900:    if (!bus_info) {
visorbus/visorchipset.c:939:    if (!bus_info) {
visorbus/visorchipset.c:962:    if (!dev_info) {
visorbus/visorchipset.c:985:    if (!visorchannel) {
visorbus/visorchipset.c:1018:   if (!dev_info) {
visorbus/visorchipset.c:1043:   if (!dev_info)
visorbus/visorchipset.c:1074:   if (!info)
visorbus/visorchipset.c:1081:   if (!payload)
visorbus/visorchipset.c:1186:   if (!req)
visorbus/visorchipset.c:1397:   if (!req)
visorbus/visorchipset.c:1527:   if (!VMCALL_SUCCESSFUL(issue_vmcall_io_controlvm_addr(&addr, &size)))
visorbus/visorchipset.c:1702:       if (!*file_controlvm_channel)
visorbus/visorchipset.c:1710:       if (!addr)
visorbus/visorchipset.c:1839:   if (!ctx) {
visorbus/visorchipset.c:1860:       if (!mapping)
visorbus/visorchipset.c:1919:       if (!parser_ctx && retry)
visorbus/visorchipset.c:1923:   if (!local_addr) {
visorbus/visorchipset.c:2028:       if (!time_after_eq(jiffies, req->expiration))
visorbus/visorchipset.c:2054:   if (!got_command) {                                          **// Useless check?**
visorbus/visorchipset.c:2118:   if (!addr)
visorbus/visorchipset.c:2123:   if (!controlvm_channel)
visorbus/visorchipset.c:2238:   if (!visorutil_spar_detect())

visorbus/vbuschannel.h:83:  if (!isprint(devinfo->devtype[0]))

visorbus/vmcallinterface.h:31:  if (!(cpuid_ecx & 0x80000000))
visorbus/vmcallinterface.h:49:  if (!(cpuid_ecx & 0x80000000))

visorhba/visorhba_main.c:142:   if (!task)
visorhba/visorhba_main.c:580:   if (!devdata)
visorhba/visorhba_main.c:587:   if (!tmpvdisk)
visorhba/visorhba_main.c:773:   if (!devdata->serverdown && !devdata->serverchangingstate) {
visorhba/visorhba_main.c:818:   if (!buf || len < NO_DISK_INQUIRY_RESULT_LEN)
visorhba/visorhba_main.c:941:           if (!scsicmd)
visorhba/visorhba_main.c:973:   if (!cmdrsp)
visorhba/visorhba_main.c:1025:  if (!devdata)
visorhba/visorhba_main.c:1057:  if (!scsihost)
visorhba/visorhba_main.c:1086:  if (!devdata->debugfs_dir) {
visorhba/visorhba_main.c:1094:  if (!devdata->debugfs_info) {
visorhba/visorhba_main.c:1151:  if (!devdata)
visorhba/visorhba_main.c:1192:  if (!visorhba_debugfs_dir)

visorinput/visorinput.c:223:    if (!devdata) {
visorinput/visorinput.c:252:    if (!devdata) {
visorinput/visorinput.c:291:    if (!visorinput_dev)
visorinput/visorinput.c:332:    if (!visorinput_dev)
visorinput/visorinput.c:376:    if (!devdata)
visorinput/visorinput.c:404:        if (!devdata->visorinput_dev)
visorinput/visorinput.c:409:        if (!devdata->visorinput_dev)
visorinput/visorinput.c:463:    if (!devdata_create(dev, devtype))
visorinput/visorinput.c:480:    if (!devdata)
visorinput/visorinput.c:579:    if (!devdata)
visorinput/visorinput.c:667:    if (!devdata) {
visorinput/visorinput.c:701:    if (!devdata) {
visorinput/visorinput.c:706:    if (!devdata->paused) {

visornic/visornic_main.c:215:           if (!count)
visornic/visornic_main.c:353:   if (!skb)
visornic/visornic_main.c:529:       if (!devdata->rcvbuf[i])
visornic/visornic_main.c:620:   if (!devdata->enab_dis_acked) {
visornic/visornic_main.c:649:   if (!netif_running(netdev)) {
visornic/visornic_main.c:972:   if (!cmdrsp)
visornic/visornic_main.c:1069:          if (!devdata->rcvbuf[i]) {
visornic/visornic_main.c:1137:  if (!(devdata->enabled && devdata->enab_dis_acked)) {
visornic/visornic_main.c:1207:          if (!prev)  /* start of list- set head */
visornic/visornic_main.c:1351:  if (!vbuf)
visornic/visornic_main.c:1507:  if (!(devdata->enabled && devdata->enab_dis_acked))
visornic/visornic_main.c:1522:          if (!devdata->rcvbuf[i]) {
visornic/visornic_main.c:1705:  if (!netdev) {
visornic/visornic_main.c:1729:  if (!devdata) {
visornic/visornic_main.c:1759:  if (!devdata->rcvbuf) {
visornic/visornic_main.c:1780:  if (!devdata->cmdrsp_rcv) {
visornic/visornic_main.c:1785:  if (!devdata->xmit_cmdrsp) {
visornic/visornic_main.c:1855:  if (!devdata->eth_debugfs_dir) {
visornic/visornic_main.c:1917:  if (!devdata) {
visornic/visornic_main.c:1930:  if (!netdev) {
visornic/visornic_main.c:1991:  if (!devdata) {
visornic/visornic_main.c:2005:  if (!devdata->server_down) {
visornic/visornic_main.c:2058:  if (!visornic_debugfs_dir)
visornic/visornic_main.c:2063:  if (!ret)
visornic/visornic_main.c:2067:  if (!ret)
ghost commented 7 years ago

Reminder: do a double-check that the changed code are in fact non-API functions.

visor_thread_stop() - clean

$ grep -rn "visor_thread_stop" *
visorhba/visorhba_main.c:138: *      visor_thread_stop - stops the thread if it is running
visorhba/visorhba_main.c:140:static void visor_thread_stop(struct task_struct *task)
visorhba/visorhba_main.c:731:   visor_thread_stop(devdata->thread);
visorhba/visorhba_main.c:1153:  visor_thread_stop(devdata->thread);

set_no_disk_inquiry_result() - clean

$ grep -rn "set_no_disk_inquiry_result" *
visorhba/visorhba_main.c:813:static int set_no_disk_inquiry_result(unsigned char *buf,
visorhba/visorhba_main.c:863:           set_no_disk_inquiry_result(buf, (size_t)cmdrsp->scsi.bufflen,

parser_id_get() - clean

$ grep -rn "parser_id_get" *
visorbus/visorchipset.c:352:parser_id_get(struct parser_context *ctx)
visorbus/visorchipset.c:901:            bus_info->partition_uuid = parser_id_get(parser_ctx);

parser_param_start() - clean

$ grep -rn "parser_param_start" *
visorbus/visorchipset.c:373:parser_param_start(struct parser_context *ctx,
visorbus/visorchipset.c:902:            parser_param_start(parser_ctx, PARSERSTRING_NAME);

parser_done() - clean

$ grep -rn "parser_done" *
visorbus/visorchipset.c:401:static void parser_done(struct parser_context *ctx)
visorbus/visorchipset.c:1854:   parser_done(ctx);
visorbus/visorchipset.c:1967:           parser_done(parser_ctx);

parser_string_get() - clean

$ grep -rn "parser_string_get" *
visorbus/visorchipset.c:408:parser_string_get(struct parser_context *ctx)
visorbus/visorchipset.c:903:            bus_info->name = parser_string_get(parser_ctx);

bus_responder() - dirty

$ grep -rn "bus_responder" *
visorbus/visorchipset.c:630:bus_responder(enum controlvm_id cmd_id,
visorbus/visorchipset.c:725:    bus_responder(cmd, pmsg_hdr, response);
visorbus/visorchipset.c:1607:   bus_responder(CONTROLVM_BUS_CREATE, bus_info->pending_msg_hdr,
visorbus/visorchipset.c:1617:   bus_responder(CONTROLVM_BUS_DESTROY, bus_info->pending_msg_hdr, 

device_responder() - dirty

$ grep -rn "device_responder" *
visorbus/visorchipset.c:666:device_responder(enum controlvm_id cmd_id,
visorbus/visorchipset.c:794:    device_responder(cmd, pmsg_hdr, response);
visorbus/visorchipset.c:1630:   device_responder(CONTROLVM_DEVICE_CREATE, dev_info->pending_msg_hdr,
visorbus/visorchipset.c:1640:   device_responder(CONTROLVM_DEVICE_DESTROY, dev_info->pending_msg_hdr,

initialize_controlvm_payload_info() - clean

$ grep -rn "initialize_controlvm_payload_info" *
visorbus/visorchipset.c:1040: * initialize_controlvm_payload_info() - init controlvm_payload_info struct
visorbus/visorchipset.c:1054:initialize_controlvm_payload_info(u64 phys_addr, u64 offset, u32 bytes,
visorbus/visorchipset.c:1107:   initialize_controlvm_payload_info(phys_addr,

visorchannel_destroy() - clean

$ grep -rn "visorchannel_destroy" *
visorbus/visorchannel.c:58:visorchannel_destroy(struct visorchannel *channel)
visorbus/visorchannel.c:467:    visorchannel_destroy(channel);
visorbus/visorbus_main.c:172:           visorchannel_destroy(dev->visorchannel);
visorbus/visorbus_main.c:1042:          visorchannel_destroy(dev->visorchannel);
visorbus/visorchipset.c:2157:   visorchannel_destroy(controlvm_channel);
visorbus/visorchipset.c:2174:   visorchannel_destroy(controlvm_channel);
visorbus/visorbus_private.h:71:void visorchannel_destroy(struct visorchannel *channel);
ghost commented 7 years ago

Branch: githubissue-103-staging-next-TurningWrenches Commit(s): e2b8be31fe978e88a4e2dfcb195ab8d3cfc8c930 65b16ed2de1e4ee42b5ef93a0544407d4c84749f 65e469e1ed9926a7406895b50e24168f8f11bd27 checkpatch-cleared: Yes T710-1 verified: Yes

ghost commented 7 years ago

++-+-+-+-+-+ + ROUND 2 + ++-+-+-+-+-+

Parameters:


$ grep -rn "if (\!.*)" drivers/staging/unisys

drivers/staging/unisys/visorbus/visorchannel.c:61: if (!channel)

drivers/staging/unisys/visorbus/visorchannel.c:322: if (!channel->needs_lock)

drivers/staging/unisys/visorbus/visorchannel.c:412: if (!channel) drivers/staging/unisys/visorbus/visorchannel.c:427: if (!channel->requested) { drivers/staging/unisys/visorbus/visorchannel.c:435: if (!channel->mapped) { drivers/staging/unisys/visorbus/visorchannel.c:460: if (!channel->requested) { drivers/staging/unisys/visorbus/visorchannel.c:469: if (!channel->mapped) {

drivers/staging/unisys/visorbus/visorbus_main.c:120: if (!drv->channel_types)

drivers/staging/unisys/visorbus/visorbus_main.c:188: if (!vdev->visorchannel)

drivers/staging/unisys/visorbus/visorbus_main.c:200: if (!vdev->visorchannel)

drivers/staging/unisys/visorbus/visorbus_main.c:212: if (!vdev->visorchannel)

drivers/staging/unisys/visorbus/visorbus_main.c:225: if (!vdev->visorchannel)

drivers/staging/unisys/visorbus/visorbus_main.c:238: if (!vdev->visorchannel)

drivers/staging/unisys/visorbus/visorbus_main.c:254: if (!vdev->visorchannel || !xbus || !xdrv) drivers/staging/unisys/visorbus/visorbus_main.c:257: if (!i)

drivers/staging/unisys/visorbus/visorbus_main.c:391: if (!channel)

drivers/staging/unisys/visorbus/visorbus_main.c:461: if (!dev->timer_active)

drivers/staging/unisys/visorbus/visorbus_main.c:714: if (!SPAR_VBUS_CHANNEL_OK_CLIENT(visorchannel_get_header(chan)))

drivers/staging/unisys/visorbus/visorbus_main.c:831: if (!visordev->device.driver) drivers/staging/unisys/visorbus/visorbus_main.c:835: if (!bdev) drivers/staging/unisys/visorbus/visorbus_main.c:838: if (!hdr_info)

Regarding the second check, the question of whether or not to remove the bus device pointer check boils down to two questions: Can we trust that the chipset_bus_no member stored in the parameter to fix_vbus_dev_info() is valid, and can we trust, in visorbus_get_device_by_id(), that just because the bus device returned by bus_find_device() is sanitized, does that mean that its parent visor_device structure is valid? It seems like we can trust both of these, so the second check will be removed.

Regarding the third check, it is not trivially provable that the bus device's visor_device struct's vbus_hdr_info member is valid. Seems reasonable to keep the check in.

drivers/staging/unisys/visorbus/visorbus_main.c:894: if (!drv->probe)

drivers/staging/unisys/visorbus/visorbus_main.c:1011: if (!hdr_info) drivers/staging/unisys/visorbus/visorbus_main.c:1021: if (!dev->debugfs_dir) { drivers/staging/unisys/visorbus/visorbus_main.c:1029: if (!dev->debugfs_client_bus_info) {

drivers/staging/unisys/visorbus/visorbus_main.c:1200: if (!dev->pausing)

drivers/staging/unisys/visorbus/visorbus_main.c:1220: if (!dev->resuming)

drivers/staging/unisys/visorbus/visorbus_main.c:1255: if (!notify_func) drivers/staging/unisys/visorbus/visorbus_main.c:1259: if (!drv) { drivers/staging/unisys/visorbus/visorbus_main.c:1278: if (!drv->pause) { drivers/staging/unisys/visorbus/visorbus_main.c:1293: if (!drv->resume) {

drivers/staging/unisys/visorbus/visorbus_main.c:1347: if (!visorbus_debugfs_dir)

drivers/staging/unisys/visorbus/visorchipset.c:356: if (!ctx)

drivers/staging/unisys/visorbus/visorchipset.c:380: if (!ctx)

drivers/staging/unisys/visorbus/visorchipset.c:408: if (!ctx)

drivers/staging/unisys/visorbus/visorchipset.c:423: if (!ctx) drivers/staging/unisys/visorbus/visorchipset.c:429: if (!pscan) drivers/staging/unisys/visorbus/visorchipset.c:439: if (!value)

drivers/staging/unisys/visorbus/visorchipset.c:652: if (!pending_msg_hdr)

drivers/staging/unisys/visorbus/visorchipset.c:670: if (!p->pending_msg_hdr)

drivers/staging/unisys/visorbus/visorchipset.c:690: if (!pending_msg_hdr)

drivers/staging/unisys/visorbus/visorchipset.c:718: if (!bus_info) { drivers/staging/unisys/visorbus/visorchipset.c:738: if (!pmsg_hdr) { drivers/staging/unisys/visorbus/visorchipset.c:756: if (!visorchannel) {

drivers/staging/unisys/visorbus/visorchipset.c:793: if (!bus_info) { drivers/staging/unisys/visorbus/visorchipset.c:808: if (!pmsg_hdr) {

drivers/staging/unisys/visorbus/visorchipset.c:845: if (!bus_info) {

drivers/staging/unisys/visorbus/visorchipset.c:898: if (!bus_info) { drivers/staging/unisys/visorbus/visorchipset.c:924: if (!dev_info) { drivers/staging/unisys/visorbus/visorchipset.c:948: if (!visorchannel) { drivers/staging/unisys/visorbus/visorchipset.c:963: if (!pmsg_hdr) {

drivers/staging/unisys/visorbus/visorchipset.c:998: if (!dev_info) { drivers/staging/unisys/visorbus/visorchipset.c:1019: if (!pmsg_hdr) {

drivers/staging/unisys/visorbus/visorchipset.c:1060: if (!dev_info) { drivers/staging/unisys/visorbus/visorchipset.c:1076: if (!pmsg_hdr) {

drivers/staging/unisys/visorbus/visorchipset.c:1114: if (!info) drivers/staging/unisys/visorbus/visorchipset.c:1121: if (!payload)

drivers/staging/unisys/visorbus/visorchipset.c:1228: if (!req)

drivers/staging/unisys/visorbus/visorchipset.c:1439: if (!req)

drivers/staging/unisys/visorbus/visorchipset.c:1569: if (!VMCALL_SUCCESSFUL(issue_vmcall_io_controlvm_addr(&addr, &size)))

drivers/staging/unisys/visorbus/visorchipset.c:1752: if (!*file_controlvm_channel) drivers/staging/unisys/visorbus/visorchipset.c:1760: if (!addr)

drivers/staging/unisys/visorbus/visorchipset.c:1889: if (!ctx) { drivers/staging/unisys/visorbus/visorchipset.c:1910: if (!mapping)

drivers/staging/unisys/visorbus/visorchipset.c:1969: if (!parser_ctx && retry) drivers/staging/unisys/visorbus/visorchipset.c:1973: if (!local_addr) {

drivers/staging/unisys/visorbus/visorchipset.c:2078: if (!time_after_eq(jiffies, req->expiration))

drivers/staging/unisys/visorbus/visorchipset.c:2104: if (!got_command) {

drivers/staging/unisys/visorbus/visorchipset.c:2168: if (!addr) drivers/staging/unisys/visorbus/visorchipset.c:2173: if (!controlvm_channel)

drivers/staging/unisys/visorbus/visorchipset.c:2294: if (!visorutil_spar_detect())

drivers/staging/unisys/visorbus/vbuschannel.h:83: if (!isprint(devinfo->devtype[0]))

drivers/staging/unisys/visorbus/vmcallinterface.h:31: if (!(cpuid_ecx & 0x80000000))

drivers/staging/unisys/visorbus/vmcallinterface.h:49: if (!(cpuid_ecx & 0x80000000))

davidker commented 7 years ago

David, you have stale branches for these (i.e. > 4 months). Do you want me to keep the branches or do we start fresh? What is your take on that.