Re: [PATCH resend] sdhci: work around broken dma boundary behaviour

[Wolfram Sang <w.sang@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3921 bytes --]


> > But we can't guarantee that. Transfer could be up to 65535 *  2K.
> 
> In sdhci.c function sdhci_prepare_data there are these checks:
> 
>     /* Sanity checks */
>     BUG_ON(data->blksz * data->blocks > 524288);
>     BUG_ON(data->blksz > host->mmc->max_blk_size);
>     BUG_ON(data->blocks > 65535);
> 
> I thought the first BUG_ON makes sure that the transfer doesn't go too big.
> Then again, I might be missing something. Is 524288 not in bytes?

Sorry, I simply missed that. Hmmm, another hard coded value :(

> > > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=28462
> > > 
> > >  Signed-off-by: Mikko Vinni <mmvinni <at> yahoo.com>
> > 
> > Proper EMail  please.
> 
> Hm, @gmail.com? (cc added for further emails)

I was just referring to using "<at>" instead of "@". The provider
doesn't really matter :)

> > > diff --git a/drivers/mmc/host/sdhci.c  b/drivers/mmc/host/sdhci.c
> > > index a25db42..8651731 100644
> > > ---  a/drivers/mmc/host/sdhci.c
> > > +++ b/drivers/mmc/host/sdhci.c
> > > @@  -1537,9 +1537,27 @@ static void sdhci_data_irq(struct sdhci_host *host, 
> >u32  intmask)
> > >           * boundaries, but  as we can't disable the feature
> > >            * we need to at least restart the transfer.
> > >            */
> > > -         if (intmask & SDHCI_INT_DMA_END)
> > >  -            sdhci_writel(host,  sdhci_readl(host, SDHCI_DMA_ADDRESS),
> > > -                  SDHCI_DMA_ADDRESS);
> > > +        if (intmask  & SDHCI_INT_DMA_END) {
> > > +             u32 dmastart, dmanow;
> > > +             dmastart =  sg_dma_address(host->data->sg);
> > 
> > This will only work for the first  512K, right?
> 
> True. If a transfer can cross more than one boundary, I suppose an
> additional variable is needed to keep track of the current state.

Yeah, thought that, too. I also wondered if we then just could not
always write our own value to DMA_ADDRESS. This is redundant on working
hardware, but the the check is not much cheaper than just doing it.

Would that change also be beyond your comfort zone?

> > > +             dmanow = sdhci_readl(host, SDHCI_DMA_ADDRESS);
> > >  +            if (dmanow ==  dmastart) {
> > > +                 /*
> > > +                  * HW failed to increase the  address.
> > > +                  * Update to the next 512KB block boundary.
> > >  +                  */
> > > +                 dmanow = (dmanow & ~0x7ffff) + 0x80000;
> > 
> > Hmm,  hardcoding these values is probably not a good idea. They should be
> > dependent  on what is written to MAKE_BLKSIZE. Maybe a common define?
> 
> Sorry, implementing that goes beyond my comfort zone. I would be happy to
> test patches, though.

I was imagining something like:

#define SDHCI_DEFAULT_BOUNDARY_SIZE	(512 * 1024)

which could be used directly in your code and later like

SDHCI_MAKE_BLKSZ(ilog2(SDHCI_DEFAULT_BOUNDARY_SIZE) - 12, ...);

(Maybe the ilog2-thingie could be another macro)

> > >  +                if  (dmanow > dmastart + host->data->blksz *
> > > +                              host->data->blocks) {
> > >  +                     WARN_ON(1);
> > > +                     dmanow =  dmastart;
> > > +                 }
> > 
> > Did this happen?
> 
> No, but I though it brings some protection in case somebody *does*
> change the boundary value without checking the code first (and happens
> to be running on flawed hardware).

So, it could go if we make that dependent if I understand correctly.

Oh, and what I forgot to say last time:

Thanks a lot for your debugging efforts! I read the bugzilla entry and
your persistency for nailing the cause is greatly appreciated. Good
work.

All the best,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]