re: mmc: Driver for CB710/720 memory card reader (MMC part)

re: mmc: Driver for CB710/720 memory card reader (MMC part)

[Dan Carpenter]

Hello Michał Mirosław,

The patch 5f5bac8272be: "mmc: Driver for CB710/720 memory card reader
(MMC part)" from May 22, 2009, leads to the following static checker
warning:

	drivers/mmc/host/cb710-mmc.c:579 cb710_mmc_set_ios()
	warn: 'err' can be either negative or positive

drivers/mmc/host/cb710-mmc.c
   112  
   113  static int cb710_check_event(struct cb710_slot *slot, u8 what)
   114  {
   115          u16 status;
   116  
   117          status = cb710_read_port_16(slot, CB710_MMC_STATUS_PORT);
   118  
   119          if (status & CB710_MMC_S0_FIFO_UNDERFLOW) {
   120                  /* it is just a guess, so log it */
   121                  dev_dbg(cb710_slot_dev(slot),
   122                          "CHECK : ignoring bit 6 in status %04X\n", status);
   123                  cb710_write_port_8(slot, CB710_MMC_STATUS0_PORT,
   124                          CB710_MMC_S0_FIFO_UNDERFLOW);
   125                  status &= ~CB710_MMC_S0_FIFO_UNDERFLOW;
   126          }
   127  
   128          if (status & CB710_MMC_STATUS_ERROR_EVENTS) {
   129                  dev_dbg(cb710_slot_dev(slot),
   130                          "CHECK : returning EIO on status %04X\n", status);
   131                  cb710_write_port_8(slot, CB710_MMC_STATUS0_PORT, status & 0xFF);
   132                  cb710_write_port_8(slot, CB710_MMC_STATUS1_PORT,
   133                          CB710_MMC_S1_RESET);
   134                  return -EIO;
   135          }
   136  
   137          /* 'what' is a bit in MMC_STATUS1 */
   138          if ((status >> 8) & what) {
   139                  cb710_write_port_8(slot, CB710_MMC_STATUS1_PORT, what);
   140                  return 1;

There are two callers to this function.  It appears to me that one
expects zero on error and one expects non-zero on error.  It is
confusing and there are no comments explaining the return codes.

   141          }
   142  
   143          return 0;
   144  }

regards,
dan carpenter

Re: mmc: Driver for CB710/720 memory card reader (MMC part)

[Michał Mirosław]

On Wed, Aug 12, 2015 at 02:00:51PM +0300, Dan Carpenter wrote:
> Hello Michał Mirosław,
> 
> The patch 5f5bac8272be: "mmc: Driver for CB710/720 memory card reader
> (MMC part)" from May 22, 2009, leads to the following static checker
> warning:
> 
> 	drivers/mmc/host/cb710-mmc.c:579 cb710_mmc_set_ios()
> 	warn: 'err' can be either negative or positive
> 
> drivers/mmc/host/cb710-mmc.c
>    112  
>    113  static int cb710_check_event(struct cb710_slot *slot, u8 what)
>    114  {
>    115          u16 status;
>    116  
>    117          status = cb710_read_port_16(slot, CB710_MMC_STATUS_PORT);
>    118  
>    119          if (status & CB710_MMC_S0_FIFO_UNDERFLOW) {
>    120                  /* it is just a guess, so log it */
>    121                  dev_dbg(cb710_slot_dev(slot),
>    122                          "CHECK : ignoring bit 6 in status %04X\n", status);
>    123                  cb710_write_port_8(slot, CB710_MMC_STATUS0_PORT,
>    124                          CB710_MMC_S0_FIFO_UNDERFLOW);
>    125                  status &= ~CB710_MMC_S0_FIFO_UNDERFLOW;
>    126          }
>    127  
>    128          if (status & CB710_MMC_STATUS_ERROR_EVENTS) {
>    129                  dev_dbg(cb710_slot_dev(slot),
>    130                          "CHECK : returning EIO on status %04X\n", status);
>    131                  cb710_write_port_8(slot, CB710_MMC_STATUS0_PORT, status & 0xFF);
>    132                  cb710_write_port_8(slot, CB710_MMC_STATUS1_PORT,
>    133                          CB710_MMC_S1_RESET);
>    134                  return -EIO;
>    135          }
>    136  
>    137          /* 'what' is a bit in MMC_STATUS1 */
>    138          if ((status >> 8) & what) {
>    139                  cb710_write_port_8(slot, CB710_MMC_STATUS1_PORT, what);
>    140                  return 1;
> 
> There are two callers to this function.  It appears to me that one
> expects zero on error and one expects non-zero on error.  It is
> confusing and there are no comments explaining the return codes.

Hah. The code is correct, though I admit that it's not obvious at first glance.
Feel free to send a patch. :-)

Call inside cb710_wait_for_event() is spinning while there's no error and the
requested event was not signalled. The call from cb710_mmc_powerup() is there
only to check for error condition and since it gets 0 in 'what' it will never
return 1 (>0).

Best Regards,
Michał Mirosław