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

[Mikko Vinni <mmvinni@yahoo.com>]

Hi,



Wolfram Sang wrote:
> I finally found some time.

Greatly appreciated.

> > Some SD host controllers (noticed on an  integrated JMicron SD reader
> > on an HP Pavilion dv5-1250eo laptop) don't  update the dma address
> > register before signaling a dma interrupt due to  a dma boundary.
> > Detect this and update the register to the next 512KB  boundary,
> > at which the transfer presumably stopped.
> > 
> > As  long as each transfer is at most 512KB in size (on this hardware
> > the max  seems to be 65536 bytes), this fix is needed at most once
> > per  transfer.
> 
> 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?

> 
> > 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)


> > 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.

> 
> > +             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.

> 
> >  +                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).

> 
> > +                 DBG("%s: next DMA  address forced "
> > +                     "from 0x%08x to  0x%08x\n",
> > +                     mmc_hostname(host->mmc), dmastart,  dmanow);
> > +             }
> > +             sdhci_writel(host, dmanow, SDHCI_DMA_ADDRESS);
> > +         }
> >  
> >           if (intmask & SDHCI_INT_DATA_END) {
> >               if (host->cmd)  {
> > -- 
> > 1.7.4.1
> 
> Regards,
> 
>    Wolfram
> 

Thanks

Mikko