> > 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/ |
signature.asc
Description: Digital signature
