[PATCH] drivers: mmc: msm: update to new arm pfn_to_dma API

[PATCH] drivers: mmc: msm: update to new arm pfn_to_dma API

[Daniel Walker]

The page_to_dma() API call was removed. It caused this build
failure,

linux-2.6/drivers/mmc/host/msm_sdcc.c: In function 'msmsdcc_config_dma':
linux-2.6/drivers/mmc/host/msm_sdcc.c:391: error: implicit declaration of function 'page_to_dma'

This updates to the new usage which is pfn_to_dma().

Signed-off-by: Daniel Walker <dwalker@codeaurora.org>
---
 drivers/mmc/host/msm_sdcc.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/msm_sdcc.c b/drivers/mmc/host/msm_sdcc.c
index a5c7049..a46b9a8 100644
--- a/drivers/mmc/host/msm_sdcc.c
+++ b/drivers/mmc/host/msm_sdcc.c
@@ -388,8 +388,9 @@ static int msmsdcc_config_dma(struct msmsdcc_host *host, struct mmc_data *data)
 		box->cmd = CMD_MODE_BOX;
 
 	/* Initialize sg dma address */
-	sg->dma_address = page_to_dma(mmc_dev(host->mmc), sg_page(sg))
-				+ sg->offset;
+	sg->dma_address = pfn_to_dma(mmc_dev(host->mmc),
+				     page_to_pfn(sg_page(sg)))
+				     + sg->offset;
 
 	if (i == (host->dma.num_ents - 1))
 			box->cmd |= CMD_LC;
-- 
1.7.0.4

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] drivers: mmc: msm: update to new arm pfn_to_dma API

[Russell King - ARM Linux]

On Tue, Jan 18, 2011 at 10:04:04AM -0800, Daniel Walker wrote:
> The page_to_dma() API call was removed. It caused this build
> failure,

Because it's an API internal interface.  Don't use it.  Why is this
driver poking about in API internals all over the place?

        for (i = 0; i < host->dma.num_ents; i++) {

This goes behind the scatterlist's back.  scatterlists may not remain
contiguous in the future.  Use the proper API.

                box->cmd = CMD_MODE_BOX;

        /* Initialize sg dma address */
        sg->dma_address = page_to_dma(mmc_dev(host->mmc), sg_page(sg))
                                + sg->offset;

        if (i == (host->dma.num_ents - 1))
                        box->cmd |= CMD_LC;

The lines above are wrongly indented.

                rows = (sg_dma_len(sg) % MCI_FIFOSIZE) ?
                        (sg_dma_len(sg) / MCI_FIFOSIZE) + 1 :
                        (sg_dma_len(sg) / MCI_FIFOSIZE) ;

                if (data->flags & MMC_DATA_READ) {
                        box->src_row_addr = msmsdcc_fifo_addr(host);
                        box->dst_row_addr = sg_dma_address(sg);

                        box->src_dst_len = (MCI_FIFOSIZE << 16) |
                                           (MCI_FIFOSIZE);
                        box->row_offset = MCI_FIFOSIZE;

                        box->num_rows = rows * ((1 << 16) + 1);
                        box->cmd |= CMD_SRC_CRCI(crci);
                } else {
                        box->src_row_addr = sg_dma_address(sg);
                        box->dst_row_addr = msmsdcc_fifo_addr(host);

                        box->src_dst_len = (MCI_FIFOSIZE << 16) |
                                           (MCI_FIFOSIZE);
                        box->row_offset = (MCI_FIFOSIZE << 16);

                        box->num_rows = rows * ((1 << 16) + 1);
                        box->cmd |= CMD_DST_CRCI(crci);
                }
                box++;
                sg++;
        }

        /* location of command block must be 64 bit aligned */
        BUG_ON(host->dma.cmd_busaddr & 0x07);

        nc->cmdptr = (host->dma.cmd_busaddr >> 3) | CMD_PTR_LP;
        host->dma.hdr.cmdptr = DMOV_CMD_PTR_LIST |
                               DMOV_CMD_ADDR(host->dma.cmdptr_busaddr);
        host->dma.hdr.complete_func = msmsdcc_dma_complete_func;

        n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
                        host->dma.num_ents, host->dma.dir);
/* dsb inside dma_map_sg will write nc out to mem as well */

That is completely broken.  Please use the official APIs - it's not
hard.  Here's how to do it correctly:

	/* location of command block must be 64 bit aligned */
	BUG_ON(host->dma.cmd_busaddr & 0x07);

	nc->cmdptr = (host->dma.cmd_busaddr >> 3) | CMD_PTR_LP;
	host->dma.hdr.cmdptr = DMOV_CMD_PTR_LIST |
			       DMOV_CMD_ADDR(host->dma.cmdptr_busaddr);
	host->dma.hdr.complete_func = msmsdcc_dma_complete_func;

	n = dma_map_sg(mmc_dev(host->mmc), host->dma.sg,
			host->dma.num_ents, host->dma.dir);
	if (n == 0)
		/* you handle mapping error here */
		/* a mapping error is not n != host->dma.num_ents */

	for_each_sg(host->dma.sg, sg, n, i) {
		if (i == n - 1)
			box->cmd |= CMD_LC;
		rows = (sg_dma_len(sg) % MCI_FIFOSIZE) ?
			(sg_dma_len(sg) / MCI_FIFOSIZE) + 1 :
			(sg_dma_len(sg) / MCI_FIFOSIZE) ;

		if (data->flags & MMC_DATA_READ) {
			box->src_row_addr = msmsdcc_fifo_addr(host);
			box->dst_row_addr = sg_dma_address(sg);

			box->src_dst_len = (MCI_FIFOSIZE << 16) |
					   (MCI_FIFOSIZE);
			box->row_offset = MCI_FIFOSIZE;

			box->num_rows = rows * ((1 << 16) + 1);
			box->cmd |= CMD_SRC_CRCI(crci);
		} else {
			box->src_row_addr = sg_dma_address(sg);
			box->dst_row_addr = msmsdcc_fifo_addr(host);

			box->src_dst_len = (MCI_FIFOSIZE << 16) |
					   (MCI_FIFOSIZE);
			box->row_offset = (MCI_FIFOSIZE << 16);

			box->num_rows = rows * ((1 << 16) + 1);
			box->cmd |= CMD_DST_CRCI(crci);
		}
		box++;
	}

and when you use writel() etc afterwards, those non-cacheable writes to
box-> will be ordered with your device write.

So that's a NAK for your original patch.

Re: [PATCH] drivers: mmc: msm: update to new arm pfn_to_dma API

[Daniel Walker]

On Tue, 2011-01-18 at 18:28 +0000, Russell King - ARM Linux wrote:
> On Tue, Jan 18, 2011 at 10:04:04AM -0800, Daniel Walker wrote:
> > The page_to_dma() API call was removed. It caused this build
> > failure,
> 
> Because it's an API internal interface.  Don't use it.  Why is this
> driver poking about in API internals all over the place?

If it's internal why is this driver able to call it?

> That is completely broken.  Please use the official APIs - it's not
> hard.  Here's how to do it correctly:

Can you make this into a patch and send it to David Brown ?

> 
> and when you use writel() etc afterwards, those non-cacheable writes to
> box-> will be ordered with your device write.
> 
> So that's a NAK for your original patch.

Are you given us alternative to my fix yet? I didn't see any of your
comments touching that area?

Daniel

-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] drivers: mmc: msm: update to new arm pfn_to_dma API

[Russell King - ARM Linux]

On Tue, Jan 18, 2011 at 12:22:14PM -0800, Daniel Walker wrote:
> On Tue, 2011-01-18 at 18:28 +0000, Russell King - ARM Linux wrote:
> > On Tue, Jan 18, 2011 at 10:04:04AM -0800, Daniel Walker wrote:
> > > The page_to_dma() API call was removed. It caused this build
> > > failure,
> > 
> > Because it's an API internal interface.  Don't use it.  Why is this
> > driver poking about in API internals all over the place?
> 
> If it's internal why is this driver able to call it?

Many things end up like that in the kernel because of the need to balance
accessibility of stuff from header files and efficiency of implementation
with access.

We could stuff them into a separate header file, move all the DMA API
implementation into a .c file, and prevent drivers from being able to
inline those functions.  They then have to have the overhead of saving
call frames onto stack, etc.  Is that something you think would be
beneficial?

But then how would we prevent drivers including that separate header
file and regaining access to them?

Answer: you can't.  So why bother making things less efficient when
there's zero benefit from doing so?

There is this comment directly above their definitions:
/*
 * dma_to_pfn/pfn_to_dma/dma_to_virt/virt_to_dma are architecture private
 * functions used internally by the DMA-mapping API to provide DMA
 * addresses. They must not be used by drivers.
 */
which was preceded by this:
/*
 * page_to_dma/dma_to_virt/virt_to_dma are architecture private functions
 * used internally by the DMA-mapping API to provide DMA addresses. They
 * must not be used by drivers.
 */

I have little sympathy for drivers breaking when they use stuff which is
explicitly documented that they should not use - and when there's proper
well known APIs (DMA-mapping) provided.

Can I also assume you've already read that comment, as you'd have had to
find the change to locate what the new function is called?

> > That is completely broken.  Please use the official APIs - it's not
> > hard.  Here's how to do it correctly:
> 
> Can you make this into a patch and send it to David Brown ?

After I've done everything else I've got to do... which could be some
time.

> > and when you use writel() etc afterwards, those non-cacheable writes to
> > box-> will be ordered with your device write.
> > 
> > So that's a NAK for your original patch.
> 
> Are you given us alternative to my fix yet? I didn't see any of your
> comments touching that area?

I'm merely reporting the fact, based on a comment in the driver.  I've
not looked any further to see how the driver works or what else it does,
or even whether it gets it right.  Do I take it from your comment that
the driver doesn't use writel et.al. ?

Re: [PATCH] drivers: mmc: msm: update to new arm pfn_to_dma API

[Daniel Walker]

On Tue, 2011-01-18 at 20:44 +0000, Russell King - ARM Linux wrote:
> On Tue, Jan 18, 2011 at 12:22:14PM -0800, Daniel Walker wrote:
> > On Tue, 2011-01-18 at 18:28 +0000, Russell King - ARM Linux wrote:
> > > On Tue, Jan 18, 2011 at 10:04:04AM -0800, Daniel Walker wrote:
> > > > The page_to_dma() API call was removed. It caused this build
> > > > failure,
> > > 
> > > Because it's an API internal interface.  Don't use it.  Why is this
> > > driver poking about in API internals all over the place?
> > 
> > If it's internal why is this driver able to call it?
> 
> Many things end up like that in the kernel because of the need to balance
> accessibility of stuff from header files and efficiency of implementation
> with access.
> 
> We could stuff them into a separate header file, move all the DMA API
> implementation into a .c file, and prevent drivers from being able to
> inline those functions.  They then have to have the overhead of saving
> call frames onto stack, etc.  Is that something you think would be
> beneficial?

I don't know .. If it's accessible to drivers you should assume they
will use it, broken or not.

> But then how would we prevent drivers including that separate header
> file and regaining access to them?
> 
> Answer: you can't.  So why bother making things less efficient when
> there's zero benefit from doing so?

That's fine, but still if it's accessible to driver you need to assume
they can/will use it (potentially) ..

> There is this comment directly above their definitions:
> /*
>  * dma_to_pfn/pfn_to_dma/dma_to_virt/virt_to_dma are architecture private
>  * functions used internally by the DMA-mapping API to provide DMA
>  * addresses. They must not be used by drivers.
>  */
> which was preceded by this:
> /*
>  * page_to_dma/dma_to_virt/virt_to_dma are architecture private functions
>  * used internally by the DMA-mapping API to provide DMA addresses. They
>  * must not be used by drivers.
>  */

That's all fine an dandy , but this driver is using them. You can't just
pretend that it's not using them, and hence break the build.

Have you considered renaming to __dma_to_pfn ?

> I have little sympathy for drivers breaking when they use stuff which is
> explicitly documented that they should not use - and when there's proper
> well known APIs (DMA-mapping) provided.

This needs to be caught in a different way, you can't just break the
driver.. AFAIK this driver went through the review process, and no one
caught this. The driver does in fact work, and is used.

> Can I also assume you've already read that comment, as you'd have had to
> find the change to locate what the new function is called?

No, I didn't read it .. I just reviewed your changes to see how to
convert to the new usage..

> > > That is completely broken.  Please use the official APIs - it's not
> > > hard.  Here's how to do it correctly:
> > 
> > Can you make this into a patch and send it to David Brown ?
> 
> After I've done everything else I've got to do... which could be some
> time.
> 
> > > and when you use writel() etc afterwards, those non-cacheable writes to
> > > box-> will be ordered with your device write.
> > > 
> > > So that's a NAK for your original patch.
> > 
> > Are you given us alternative to my fix yet? I didn't see any of your
> > comments touching that area?
> 
> I'm merely reporting the fact, based on a comment in the driver.  I've
> not looked any further to see how the driver works or what else it does,
> or even whether it gets it right.  Do I take it from your comment that
> the driver doesn't use writel et.al. ?

Just greping the driver I found at least one writel() .. I'm not really
concerned with that tho. I use this driver, I need this driver to work.
You broke the driver, and your NAK'ing the fix ..

We can fix all the issues that your bringing up, but the driver should
still work while we're doing that.

Daniel


-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] drivers: mmc: msm: update to new arm pfn_to_dma API

[Russell King - ARM Linux]

On Tue, Jan 18, 2011 at 01:00:20PM -0800, Daniel Walker wrote:
> Just greping the driver I found at least one writel() .. I'm not really
> concerned with that tho. I use this driver, I need this driver to work.
> You broke the driver, and your NAK'ing the fix ..

If drivers use documented internal APIs, they _will_ break, and that's
tough luck.  The advertised API (DMA-mapping) is what we guarantee to
support and fixup drivers for.

If you don't know that a driver is using an internal API then you can't
preempt it breaking.  The answer is drivers must not use internal APIs.

Had the driver been properly reviewed before being merged, it should
have been caught before it was merged.

I've given you the outline of a proper fix, and you've probably already
spent longer discussing it than it would take you to cut'n'paste the
code into kernel and test it...

Re: [PATCH] drivers: mmc: msm: update to new arm pfn_to_dma API

[Daniel Walker]

On Tue, 2011-01-18 at 21:16 +0000, Russell King - ARM Linux wrote:
> On Tue, Jan 18, 2011 at 01:00:20PM -0800, Daniel Walker wrote:
> > Just greping the driver I found at least one writel() .. I'm not really
> > concerned with that tho. I use this driver, I need this driver to work.
> > You broke the driver, and your NAK'ing the fix ..
> 
> If drivers use documented internal APIs, they _will_ break, and that's
> tough luck.  The advertised API (DMA-mapping) is what we guarantee to
> support and fixup drivers for.

I don't think that's a good policy. There could be reasons why a driver
might use an internal API .. Breaking the build is worse, to me, than
using an internal API.

> If you don't know that a driver is using an internal API then you can't
> preempt it breaking.  The answer is drivers must not use internal APIs.

At least you need to be receptive to fixes ..

> Had the driver been properly reviewed before being merged, it should
> have been caught before it was merged.

I didn't merge it, so I don't know how it was merged .. Bottom line is
it's a stable kernel driver.

> I've given you the outline of a proper fix, and you've probably already
> spent longer discussing it than it would take you to cut'n'paste the
> code into kernel and test it...

Did you give us a fix for this issue? That's what I was asking in the
last email cause it didn't appear like you touched that area.

Daniel


-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] drivers: mmc: msm: update to new arm pfn_to_dma API

[Russell King - ARM Linux]

On Tue, Jan 18, 2011 at 01:28:58PM -0800, Daniel Walker wrote:
> On Tue, 2011-01-18 at 21:16 +0000, Russell King - ARM Linux wrote:
> > On Tue, Jan 18, 2011 at 01:00:20PM -0800, Daniel Walker wrote:
> > > Just greping the driver I found at least one writel() .. I'm not really
> > > concerned with that tho. I use this driver, I need this driver to work.
> > > You broke the driver, and your NAK'ing the fix ..
> > 
> > If drivers use documented internal APIs, they _will_ break, and that's
> > tough luck.  The advertised API (DMA-mapping) is what we guarantee to
> > support and fixup drivers for.
> 
> I don't think that's a good policy. There could be reasons why a driver
> might use an internal API .. Breaking the build is worse, to me, than
> using an internal API.

If things use the wrong APIs they get broken, plain and simple.  It
happens, and we cope with it and fix it properly when it happens,
rather than subsituting the wrong way for another wrong way.

> Did you give us a fix for this issue? That's what I was asking in the
> last email cause it didn't appear like you touched that area.

I gave you an example of how to fix the problem you reported - what
more do you want?