[PATCH] RFC: mtd/spi-nor: add checking for the spi_nor_read

[Hou Zhiqiang]

Hi Michal,

> -----Original Message-----
> From: Michal Suchanek [mailto:hramrach at gmail.com]
> Sent: 2015年8月14日 12:43
> To: Hou Zhiqiang-B48286
> Cc: linux-mtd at lists.infradead.org; computersforpeace at gmail.com;
> dwmw2 at infradead.org; jonatas.rech at datacom.ind.br; Jane.Wan at gainspeed.com;
> shijie.huang at intel.com; valentin.longchamp at keymile.com;
> hkallweit1 at gmail.com; beanhuo at micron.com; Hu Mingkai-B21284
> Subject: Re: [PATCH] RFC: mtd/spi-nor: add checking for the spi_nor_read
> 
> On 14 August 2015 at 06:30, Michal Suchanek <hramrach at gmail.com> wrote:
> > On 14 August 2015 at 05:40, Hou Zhiqiang <B48286 at freescale.com> wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: Michal Suchanek [mailto:hramrach at gmail.com]
> >>> Sent: 2015年8月13日 17:42
> >>> To: Hou Zhiqiang-B48286
> >>> Cc: linux-mtd at lists.infradead.org; computersforpeace at gmail.com;
> >>> dwmw2 at infradead.org; jonatas.rech at datacom.ind.br;
> >>> Jane.Wan at gainspeed.com; shijie.huang at intel.com;
> >>> valentin.longchamp at keymile.com; hkallweit1 at gmail.com;
> >>> beanhuo at micron.com; Hu Mingkai-B21284
> >>> Subject: Re: [PATCH] RFC: mtd/spi-nor: add checking for the
> >>> spi_nor_read
> >>>
> >>> On 13 August 2015 at 11:03, Hou Zhiqiang <B48286 at freescale.com> wrote:
> >>> > Hello,
> >>> >
> >>> > I want to add the 4-Byte addressing flashes support for the
> >>> > Freescale eSPI controller dirver. Appreciate your suggestions.
> >>> >
> >>> > Background:
> >>> > In the Freescale eSPI controller dirver, if the transaction length
> >>> > exceed 64KiB, the contents of the transaction was touched to
> >>> > update the command with assumed the 3-Byte address width. It is a
> >>> > workaround, due to the Freescale eSPI controller have a hardware
> >>> > limit that the maximum one-time transaction length is 64KiB, that
> >>> > isn't consistent
> >>> with many other vendors'
> >>> > SPI controllers, and so far the SPI flash driver assume the SPI
> >>> > controller have the ability to complete the specified transaction
> >>> length at one-time.
> >>> >
> >>> > But this workaround is only suit for 3-Byte addressing slaves, as
> >>> > time goes by, there are 4-Byte address width SPI flashes, so this
> >>> > assumption isn't correct and this workaround discombobulate the
> >>> > controller driver layer and protocol driver layer. So, this
> >>> > workaround should be removed and the SPI client driver should
> >>> > ensure the
> >>> transaction completion.
> >>> >
> >>> > There are two solutions:
> >>> > 1. In spi-nor framework, compare the retlen with the transaction
> >>> > length specified, if the retlen less than the transaction length
> >>> > and with no error number returned, re-initiate the transaction
> >>> > with the
> >>> updated address.
> >>> > Advantage:
> >>> > It won't affect other SPI controllers without this limit.
> >>> > Disadvantage:
> >>> > It is not a systematic solution.
> >>>
> >>> This can probably break transactions that are performed on non-flash
> >>> slaves but look like flash read command.
> >>>
> >>> >
> >>> > 2. Add quirk support, something like i2c quirk.
> >>> > Advantage:
> >>> > It is a systematic solution.
> >>> > Disadvantage:
> >>> > The quirk mechanism is only useful for Freescale eSPI controller,
> >>> > but it will affect whole SPI framework.
> >>>
> >>> I added this quirk in m25p80.c since my current SPI master driver is
> >>> just broken so I tried to work around that. While the quirk itself
> >>> is rejected as non-syetematic this series that adds the required
> >>> support for checking the return value of the SPI master driver
> transfer functions.
> >>>
> >>> So you have options:
> >>>
> >>> 2a: add SPI master quirk and check it in m25p80.c and truncate the
> >>> transfer in m25p80.c
> >>>
> >>> 2b: truncate the transfer in SPI master driver and just return the
> >>> number of bytes transferred
> >>>
> >>
> >> I expect this way, so the spi slave driver should check the retlen
> >> with the transaction length and do some loop transfer, and make sure
> >> getting all data this transaction wanted.
> >>
> >>> This patch series should handle fragmenting the transfers in
> >>> spi-nor.c
> >>>
> >>> http://lists.infradead.org/pipermail/linux-mtd/2015-July/060466.html
> >>>
> >>
> >> Please correct me if I'm wrong, AFAIU the mtd layer will check the
> >> retlen and if it isn't equal to the transaction length, the EIO will
> be returned.
> >
> > I never had this happen because with this patch series spi-nor checks
> > the amount of data transferred and continues until the whole length is
> > transferred. Without it just sets retlen to length regardless of
> > amount of data transferred.
> >
> 
> Looking at the code closely mtdcore just invokes _read in mtd_read and
> passes the result on. mtdchar does the same. ubi has assert that read
> amount of data is equal requested so if you used ubi you whould have to
> fix it or add read loop in spi-nor to work around ubi deficiency.
> 

But in mtdblock.c, mtd_read is invoked by mtdblock_readsect->do_cached_read
without read loop. And just compare the retlen with requested data size, if
not equal, the EIO will be reported.

243                         ret = mtd_read(mtd, pos, size, &retlen, buf);
244                         if (ret)
245                                 return ret;
246                         if (retlen != size)
247                                 return -EIO;
 

> Thanks
> 
> Michal