re: mmc: sh_mmcif: remove now superfluous sh_mmcif_host::data member

re: mmc: sh_mmcif: remove now superfluous sh_mmcif_host::data member

[Dan Carpenter]

Hello Guennadi Liakhovetski,

This is a semi-automatic email about new static checker warnings.

The patch 699834045f1e: "mmc: sh_mmcif: remove now superfluous 
sh_mmcif_host::data member" from Dec 26, 2011, leads to the following 
Smatch complaint:

drivers/mmc/host/sh_mmcif.c:822 sh_mmcif_set_cmd()
	 error: we previously assumed 'data' could be null (see line 787)

drivers/mmc/host/sh_mmcif.c
   786		/* WDAT / DATW */
   787		if (data) {
                    ^^^^
Check.

   788			tmp |= CMD_SET_WDAT;
   789			switch (host->bus_width) {
   790			case MMC_BUS_WIDTH_1:
   791				tmp |= CMD_SET_DATW_1;
   792				break;
   793			case MMC_BUS_WIDTH_4:
   794				tmp |= CMD_SET_DATW_4;
   795				break;
   796			case MMC_BUS_WIDTH_8:
   797				tmp |= CMD_SET_DATW_8;
   798				break;
   799			default:
   800				dev_err(&host->pd->dev, "Unsupported bus width.\n");
   801				break;
   802			}
   803			switch (host->timing) {
   804			case MMC_TIMING_UHS_DDR50:
   805				/*
   806				 * MMC core will only set this timing, if the host
   807				 * advertises the MMC_CAP_UHS_DDR50 capability. MMCIF
   808				 * implementations with this capability, e.g. sh73a0,
   809				 * will have to set it in their platform data.
   810				 */
   811				tmp |= CMD_SET_DARS;
   812				break;
   813			}
   814		}
   815		/* DWEN */
   816		if (opc == MMC_WRITE_BLOCK || opc == MMC_WRITE_MULTIPLE_BLOCK)
   817			tmp |= CMD_SET_DWEN;
   818		/* CMLTE/CMD12EN */
   819		if (opc == MMC_READ_MULTIPLE_BLOCK || opc == MMC_WRITE_MULTIPLE_BLOCK) {
   820			tmp |= CMD_SET_CMLTE | CMD_SET_CMD12EN;
   821			sh_mmcif_bitset(host, MMCIF_CE_BLOCK_SET,
   822					data->blocks << 16);
                                        ^^^^^^^^^^^^
Dereference.

We used to assume that host->data might be NULL but that mrq->data was
a valid pointer.  But now they are unified into one pointer.

   823		}
   824		/* RIDXC[1:0] check bits */

regards,
dan carpenter

Re: *** GMX Spamverdacht *** re: mmc: sh_mmcif: remove now superfluous sh_mmcif_host::data member

[Guennadi Liakhovetski]

Hi Dan,

On Wed, 6 Nov 2013, Dan Carpenter wrote:

> Hello Guennadi Liakhovetski,
> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch 699834045f1e: "mmc: sh_mmcif: remove now superfluous 
> sh_mmcif_host::data member" from Dec 26, 2011, leads to the following 
> Smatch complaint:
> 
> drivers/mmc/host/sh_mmcif.c:822 sh_mmcif_set_cmd()
> 	 error: we previously assumed 'data' could be null (see line 787)
> 
> drivers/mmc/host/sh_mmcif.c
>    786		/* WDAT / DATW */
>    787		if (data) {
>                     ^^^^
> Check.
> 
>    788			tmp |= CMD_SET_WDAT;
>    789			switch (host->bus_width) {
>    790			case MMC_BUS_WIDTH_1:
>    791				tmp |= CMD_SET_DATW_1;
>    792				break;
>    793			case MMC_BUS_WIDTH_4:
>    794				tmp |= CMD_SET_DATW_4;
>    795				break;
>    796			case MMC_BUS_WIDTH_8:
>    797				tmp |= CMD_SET_DATW_8;
>    798				break;
>    799			default:
>    800				dev_err(&host->pd->dev, "Unsupported bus width.\n");
>    801				break;
>    802			}
>    803			switch (host->timing) {
>    804			case MMC_TIMING_UHS_DDR50:
>    805				/*
>    806				 * MMC core will only set this timing, if the host
>    807				 * advertises the MMC_CAP_UHS_DDR50 capability. MMCIF
>    808				 * implementations with this capability, e.g. sh73a0,
>    809				 * will have to set it in their platform data.
>    810				 */
>    811				tmp |= CMD_SET_DARS;
>    812				break;
>    813			}
>    814		}
>    815		/* DWEN */
>    816		if (opc == MMC_WRITE_BLOCK || opc == MMC_WRITE_MULTIPLE_BLOCK)
>    817			tmp |= CMD_SET_DWEN;
>    818		/* CMLTE/CMD12EN */
>    819		if (opc == MMC_READ_MULTIPLE_BLOCK || opc == MMC_WRITE_MULTIPLE_BLOCK) {
>    820			tmp |= CMD_SET_CMLTE | CMD_SET_CMD12EN;
>    821			sh_mmcif_bitset(host, MMCIF_CE_BLOCK_SET,
>    822					data->blocks << 16);
>                                         ^^^^^^^^^^^^
> Dereference.
> 
> We used to assume that host->data might be NULL but that mrq->data was
> a valid pointer.  But now they are unified into one pointer.

Yes, formally this isn't correct, but in practice this is valid, because 
for those two opcodes, handled in this "if" data cannot be NULL.

Thanks
Guennadi

> 
>    823		}
>    824		/* RIDXC[1:0] check bits */
> 
> regards,
> dan carpenter
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: *** GMX Spamverdacht *** re: mmc: sh_mmcif: remove now superfluous sh_mmcif_host::data member

[Dan Carpenter]

On Tue, Dec 03, 2013 at 10:48:58PM +0100, Guennadi Liakhovetski wrote:
> > 
> > We used to assume that host->data might be NULL but that mrq->data was
> > a valid pointer.  But now they are unified into one pointer.
> 
> Yes, formally this isn't correct, but in practice this is valid, because 
> for those two opcodes, handled in this "if" data cannot be NULL.
> 

Fine fine.  I suspected it might be something like that.  We only send
the warning messages one time so no worries.

regards,
dan carpenter