should struct device.dma_mask still be a pointer?

should struct device.dma_mask still be a pointer?

[Uwe Kleine-König]

Hello,

IMHO it's strange that struct device.dma_mask is a pointer instead of a
plain u64.  The reason this was done back then is described in
8ab1bc19e974fdebe76c065fe444979c84ba2f74[1]:

	Attached is a patch which moves dma_mask into struct device and cleans up the
	scsi mid-layer to use it (instead of using struct pci_dev).  The advantage to
	doing this is probably most apparent on non-pci bus architectures where
	currently you have to construct a fake pci_dev just so you can get the bounce
	buffers to work correctly.

	The patch tries to perturb the minimum amount of code, so dma_mask in struct
	device is simply a pointer to the one in pci_dev.  However, it will make it
	easy for me now to add generic device to MCA without having to go the fake pci
	route.

As I work on such a non-pci bus architecture it's still ugly that this
is a pointer because I have to allocate extra memory for that.

Is there a reason not to get rid of struct pci_dev.dma_mask and use
struct pci_dev.dev.dma_mask instead?  (Well apart from the needed
effort of course.)

If not, the following would be needed:

	- remove struct pci.dma_mask
	- make struct device.dma_mask an u64 (instead of u64*)
	- substitue var.dma_mask by var.dev.dma_mask for all
	  struct pci_dev var
	- substitue var.dma_mask by &(var.dma_mask) for all
	  struct device var

and note that there are statically initialized struct device (and maybe
struct pci_dev?) that need fixing, too.  (e.g.
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/mach-mx2/devices.c;h=a0aeb8a4adc19ef419a0a045ad3b882131597106;hb=HEAD#l265
)

Additionally this could be done for struct device.dma_parms.

Julia: help!

Best regards
Uwe

[1] as this is pre-2.6.12 it's only available in tglx' histroy tree,
e.g.  http://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=commitdiff;h=8ab1bc19e974fdebe76c065fe444979c84ba2f74

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: should struct device.dma_mask still be a pointer?

[Konrad Rzeszutek Wilk]

On Tue, Jun 22, 2010 at 12:52:33PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> IMHO it's strange that struct device.dma_mask is a pointer instead of a
> plain u64.  The reason this was done back then is described in
> 8ab1bc19e974fdebe76c065fe444979c84ba2f74[1]:
> 
> 	Attached is a patch which moves dma_mask into struct device and cleans up the
> 	scsi mid-layer to use it (instead of using struct pci_dev).  The advantage to
> 	doing this is probably most apparent on non-pci bus architectures where
> 	currently you have to construct a fake pci_dev just so you can get the bounce
> 	buffers to work correctly.
> 
> 	The patch tries to perturb the minimum amount of code, so dma_mask in struct
> 	device is simply a pointer to the one in pci_dev.  However, it will make it
> 	easy for me now to add generic device to MCA without having to go the fake pci
> 	route.
> 
> As I work on such a non-pci bus architecture it's still ugly that this
> is a pointer because I have to allocate extra memory for that.
> 
> Is there a reason not to get rid of struct pci_dev.dma_mask and use
> struct pci_dev.dev.dma_mask instead?  (Well apart from the needed
> effort of course.)

Lets CC Fujita. He has been redoing some of the DMA API, and making the
PCI DMA API be used in favour of the old DMA API.

But from the sounds of it for your architecture you need a DMA API, not
a PCI DMA and you want to merge the dma_mask in one? Preferably in the
struct device one?

Re: should struct device.dma_mask still be a pointer?

[FUJITA Tomonori]

> IMHO it's strange that struct device.dma_mask is a pointer instead of a
> plain u64.  The reason this was done back then is described in
> 8ab1bc19e974fdebe76c065fe444979c84ba2f74[1]:
> 
> 	Attached is a patch which moves dma_mask into struct device and cleans up the
> 	scsi mid-layer to use it (instead of using struct pci_dev).  The advantage to
> 	doing this is probably most apparent on non-pci bus architectures where
> 	currently you have to construct a fake pci_dev just so you can get the bounce
> 	buffers to work correctly.
> 
> 	The patch tries to perturb the minimum amount of code, so dma_mask in struct
> 	device is simply a pointer to the one in pci_dev.  However, it will make it
> 	easy for me now to add generic device to MCA without having to go the fake pci
> 	route.

Yeah, that's a strange design. As the commit log said, it's due to the
historical reason. We invented the pci dma model first then moved to
the generic dma model.


> As I work on such a non-pci bus architecture it's still ugly that this
> is a pointer because I have to allocate extra memory for that.

The popular trick to avoid allocating the extra memory for that is:

device.dma_mask = &device.coherent_dma_mask;


> Is there a reason not to get rid of struct pci_dev.dma_mask and use
> struct pci_dev.dev.dma_mask instead?  (Well apart from the needed
> effort of course.)
> 
> If not, the following would be needed:
> 
> 	- remove struct pci.dma_mask
> 	- make struct device.dma_mask an u64 (instead of u64*)
> 	- substitue var.dma_mask by var.dev.dma_mask for all
> 	  struct pci_dev var
> 	- substitue var.dma_mask by &(var.dma_mask) for all
> 	  struct device var
> 
> and note that there are statically initialized struct device (and maybe
> struct pci_dev?) that need fixing, too.  (e.g.
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/mach-mx2/devices.c;h=a0aeb8a4adc19ef419a0a045ad3b882131597106;hb=HEAD#l265
> )

That's exactly the perturbation that the commit log refers to.

We need to modify all the struct device at a time. We could, however,
I don't think that it's worth doing. Little gain.


> Additionally this could be done for struct device.dma_parms.

Yeah, we should have all the dma parameters in dma_parms.

Re: should struct device.dma_mask still be a pointer?

[Uwe Kleine-König]

Hello,

I'm picking up this old thread because I'm currently faced again with
this problem ... and I'm really annoyed about it.

On Thu, Jul 01, 2010 at 10:35:58AM +0900, FUJITA Tomonori wrote:
> > IMHO it's strange that struct device.dma_mask is a pointer instead of a
> > plain u64.  The reason this was done back then is described in
> > 8ab1bc19e974fdebe76c065fe444979c84ba2f74[1]:
> > 
> > 	Attached is a patch which moves dma_mask into struct device and cleans up the
> > 	scsi mid-layer to use it (instead of using struct pci_dev).  The advantage to
> > 	doing this is probably most apparent on non-pci bus architectures where
> > 	currently you have to construct a fake pci_dev just so you can get the bounce
> > 	buffers to work correctly.
> > 
> > 	The patch tries to perturb the minimum amount of code, so dma_mask in struct
> > 	device is simply a pointer to the one in pci_dev.  However, it will make it
> > 	easy for me now to add generic device to MCA without having to go the fake pci
> > 	route.
> 
> Yeah, that's a strange design. As the commit log said, it's due to the
> historical reason. We invented the pci dma model first then moved to
> the generic dma model.
> 
> 
> > As I work on such a non-pci bus architecture it's still ugly that this
> > is a pointer because I have to allocate extra memory for that.
> 
> The popular trick to avoid allocating the extra memory for that is:
> 
> device.dma_mask = &device.coherent_dma_mask;
Does this work in general?  Or are there any corner cases that make this
trick fail?

> > Is there a reason not to get rid of struct pci_dev.dma_mask and use
> > struct pci_dev.dev.dma_mask instead?  (Well apart from the needed
> > effort of course.)
> > 
> > If not, the following would be needed:
> > 
> > 	- remove struct pci.dma_mask
> > 	- make struct device.dma_mask an u64 (instead of u64*)
> > 	- substitue var.dma_mask by var.dev.dma_mask for all
> > 	  struct pci_dev var
> > 	- substitue var.dma_mask by &(var.dma_mask) for all
> > 	  struct device var
> > 
> > and note that there are statically initialized struct device (and maybe
> > struct pci_dev?) that need fixing, too.  (e.g.
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/mach-mx2/devices.c;h=a0aeb8a4adc19ef419a0a045ad3b882131597106;hb=HEAD#l265
> > )
> 
> That's exactly the perturbation that the commit log refers to.
> 
> We need to modify all the struct device at a time. We could, however,
> I don't think that it's worth doing. Little gain.
> 
> 
> > Additionally this could be done for struct device.dma_parms.
> 
> Yeah, we should have all the dma parameters in dma_parms.
That applies to dma_mask and coherent_dma_mask, too, I assume?

Instead of converting all at a time, what about adding an
u64 dma_mask_real to struct device (assuming coherent_dma_mask cannot be
used for it) and use this if dma_mask is NULL.  For me it would make
live a bit easier, because for some time I could just use
device.dma_mask = &device.dma_mask_real instead of allocating an u64
dynamically.  Together with some accessor functions and deprecating
direct access to the dma related members of struct device the drivers
and architectures could be converted one after another.  The final step
to get rid of the pointers would be small then.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: should struct device.dma_mask still be a pointer?

[FUJITA Tomonori]

On Tue, 2 Nov 2010 11:41:04 +0100
Uwe Kleine-K^[$(D+S^[(Bnig <u.kleine-koenig@pengutronix.de> wrote:

> > > As I work on such a non-pci bus architecture it's still ugly that this
> > > is a pointer because I have to allocate extra memory for that.
> > 
> > The popular trick to avoid allocating the extra memory for that is:
> > 
> > device.dma_mask = &device.coherent_dma_mask;
> Does this work in general?  Or are there any corner cases that make this
> trick fail?

It doesn't work if the coherent dma mask of a device is not same as
the dma mask of the device.


> > > Is there a reason not to get rid of struct pci_dev.dma_mask and use
> > > struct pci_dev.dev.dma_mask instead?  (Well apart from the needed
> > > effort of course.)
> > > 
> > > If not, the following would be needed:
> > > 
> > > 	- remove struct pci.dma_mask
> > > 	- make struct device.dma_mask an u64 (instead of u64*)
> > > 	- substitue var.dma_mask by var.dev.dma_mask for all
> > > 	  struct pci_dev var
> > > 	- substitue var.dma_mask by &(var.dma_mask) for all
> > > 	  struct device var
> > > 
> > > and note that there are statically initialized struct device (and maybe
> > > struct pci_dev?) that need fixing, too.  (e.g.
> > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/mach-mx2/devices.c;h=a0aeb8a4adc19ef419a0a045ad3b882131597106;hb=HEAD#l265
> > > )
> > 
> > That's exactly the perturbation that the commit log refers to.
> > 
> > We need to modify all the struct device at a time. We could, however,
> > I don't think that it's worth doing. Little gain.
> > 
> > 
> > > Additionally this could be done for struct device.dma_parms.
> > 
> > Yeah, we should have all the dma parameters in dma_parms.
> That applies to dma_mask and coherent_dma_mask, too, I assume?

Yes.


> Instead of converting all at a time, what about adding an
> u64 dma_mask_real to struct device (assuming coherent_dma_mask cannot be
> used for it) and use this if dma_mask is NULL.  For me it would make
> live a bit easier, because for some time I could just use
> device.dma_mask = &device.dma_mask_real instead of allocating an u64
> dynamically.  Together with some accessor functions and deprecating
> direct access to the dma related members of struct device the drivers
> and architectures could be converted one after another.  The final step
> to get rid of the pointers would be small then.

But after we finish the above, after all, we still have dma_mask in
device strcuture. As I said before, we should move dma stuff to
dma_params.

I'm not sure why this really troubles you. Can you give me a pointer
to what you have been working on? You have been working on non pci
device, right? Why can't you do like pci_dev, embedding
device_dma_parameters to your own device structure.

Re: should struct device.dma_mask still be a pointer?

[Uwe Kleine-König]

Hello,

On Tue, Nov 02, 2010 at 10:03:32PM +0900, FUJITA Tomonori wrote:
> On Tue, 2 Nov 2010 11:41:04 +0100
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> 
> > > > As I work on such a non-pci bus architecture it's still ugly that this
> > > > is a pointer because I have to allocate extra memory for that.
> > > 
> > > The popular trick to avoid allocating the extra memory for that is:
> > > 
> > > device.dma_mask = &device.coherent_dma_mask;
> > Does this work in general?  Or are there any corner cases that make this
> > trick fail?
> 
> It doesn't work if the coherent dma mask of a device is not same as
> the dma mask of the device.
> 
> 
> > > > Is there a reason not to get rid of struct pci_dev.dma_mask and use
> > > > struct pci_dev.dev.dma_mask instead?  (Well apart from the needed
> > > > effort of course.)
> > > > 
> > > > If not, the following would be needed:
> > > > 
> > > > 	- remove struct pci.dma_mask
> > > > 	- make struct device.dma_mask an u64 (instead of u64*)
> > > > 	- substitue var.dma_mask by var.dev.dma_mask for all
> > > > 	  struct pci_dev var
> > > > 	- substitue var.dma_mask by &(var.dma_mask) for all
> > > > 	  struct device var
> > > > 
> > > > and note that there are statically initialized struct device (and maybe
> > > > struct pci_dev?) that need fixing, too.  (e.g.
> > > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/mach-mx2/devices.c;h=a0aeb8a4adc19ef419a0a045ad3b882131597106;hb=HEAD#l265
> > > > )
> > > 
> > > That's exactly the perturbation that the commit log refers to.
> > > 
> > > We need to modify all the struct device at a time. We could, however,
> > > I don't think that it's worth doing. Little gain.
> > > 
> > > 
> > > > Additionally this could be done for struct device.dma_parms.
> > > 
> > > Yeah, we should have all the dma parameters in dma_parms.
> > That applies to dma_mask and coherent_dma_mask, too, I assume?
> 
> Yes.
> 
> 
> > Instead of converting all at a time, what about adding an
> > u64 dma_mask_real to struct device (assuming coherent_dma_mask cannot be
> > used for it) and use this if dma_mask is NULL.  For me it would make
> > live a bit easier, because for some time I could just use
> > device.dma_mask = &device.dma_mask_real instead of allocating an u64
> > dynamically.  Together with some accessor functions and deprecating
> > direct access to the dma related members of struct device the drivers
> > and architectures could be converted one after another.  The final step
> > to get rid of the pointers would be small then.
> 
> But after we finish the above, after all, we still have dma_mask in
> device strcuture. As I said before, we should move dma stuff to
> dma_params.
After we finished the above it's quite easy to move everything into
dma_parms.  (At least if it's not a pointer, that then again needs an
additional allocation.)
 
> I'm not sure why this really troubles you. Can you give me a pointer
> to what you have been working on? You have been working on non pci
> device, right? Why can't you do like pci_dev, embedding
> device_dma_parameters to your own device structure.
I'm changing the way imx (ARCH=arm) registers its devices.  Currently
we have in arch/arm/mach-imx/devices.c:

	static u64 imx1_camera_dmamask = DMA_BIT_MASK(32);

	struct platform_device imx1_camera_device = {
		...
		.dev = {
			.dma_mask = &imx1_camera_dmamask,
			.coherent_dma_mask = DMA_BIT_MASK(32),
		},
		...
	}

and I want to make registration dynamic (e.g. using
platform_device_register_resndata, see
arch/arm/plat-mxc/devices/platform-imx-i2c.c for an example).

Currently I have a function imx_add_platform_device (that does the same
as platform_device_register_resndata[1]) and now I want to register a
device that needs .dma_mask set, so I added something like:

	if (dmamask) {
                /*
                 * This memory isn't freed when the device is put,
                 * I don't have a nice idea for that though.  Conceptually
                 * dma_mask in struct device should not be a pointer.
                 * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
                 */
                pdev->dev.dma_mask =
                        kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
                if (!pdev->dev.dma_mask)
                        goto err;

                *pdev->dev.dma_mask = dmamask;
                pdev->dev.coherent_dma_mask = dmamask;
	}

So there is no private struct I could extend easily.  And I prefer
cleaning up global oddities instead of being the x-th person to work
around it.

Best regards
Uwe

[1] platform_device_register_resndata is newer and the result of making
imx_add_platform_device global :-)

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: should struct device.dma_mask still be a pointer?

[FUJITA Tomonori]

On Tue, 2 Nov 2010 14:45:11 +0100
Uwe Kleine-K^[$(D+S^[(Bnig <u.kleine-koenig@pengutronix.de> wrote:

> Hello,
> 
> On Tue, Nov 02, 2010 at 10:03:32PM +0900, FUJITA Tomonori wrote:
> > On Tue, 2 Nov 2010 11:41:04 +0100
> > Uwe Kleine-K^[$(D+S^[(Bnig <u.kleine-koenig@pengutronix.de> wrote:
> > 
> > > > > As I work on such a non-pci bus architecture it's still ugly that this
> > > > > is a pointer because I have to allocate extra memory for that.
> > > > 
> > > > The popular trick to avoid allocating the extra memory for that is:
> > > > 
> > > > device.dma_mask = &device.coherent_dma_mask;
> > > Does this work in general?  Or are there any corner cases that make this
> > > trick fail?
> > 
> > It doesn't work if the coherent dma mask of a device is not same as
> > the dma mask of the device.
> > 
> > 
> > > > > Is there a reason not to get rid of struct pci_dev.dma_mask and use
> > > > > struct pci_dev.dev.dma_mask instead?  (Well apart from the needed
> > > > > effort of course.)
> > > > > 
> > > > > If not, the following would be needed:
> > > > > 
> > > > > 	- remove struct pci.dma_mask
> > > > > 	- make struct device.dma_mask an u64 (instead of u64*)
> > > > > 	- substitue var.dma_mask by var.dev.dma_mask for all
> > > > > 	  struct pci_dev var
> > > > > 	- substitue var.dma_mask by &(var.dma_mask) for all
> > > > > 	  struct device var
> > > > > 
> > > > > and note that there are statically initialized struct device (and maybe
> > > > > struct pci_dev?) that need fixing, too.  (e.g.
> > > > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/mach-mx2/devices.c;h=a0aeb8a4adc19ef419a0a045ad3b882131597106;hb=HEAD#l265
> > > > > )
> > > > 
> > > > That's exactly the perturbation that the commit log refers to.
> > > > 
> > > > We need to modify all the struct device at a time. We could, however,
> > > > I don't think that it's worth doing. Little gain.
> > > > 
> > > > 
> > > > > Additionally this could be done for struct device.dma_parms.
> > > > 
> > > > Yeah, we should have all the dma parameters in dma_parms.
> > > That applies to dma_mask and coherent_dma_mask, too, I assume?
> > 
> > Yes.
> > 
> > 
> > > Instead of converting all at a time, what about adding an
> > > u64 dma_mask_real to struct device (assuming coherent_dma_mask cannot be
> > > used for it) and use this if dma_mask is NULL.  For me it would make
> > > live a bit easier, because for some time I could just use
> > > device.dma_mask = &device.dma_mask_real instead of allocating an u64
> > > dynamically.  Together with some accessor functions and deprecating
> > > direct access to the dma related members of struct device the drivers
> > > and architectures could be converted one after another.  The final step
> > > to get rid of the pointers would be small then.
> > 
> > But after we finish the above, after all, we still have dma_mask in
> > device strcuture. As I said before, we should move dma stuff to
> > dma_params.
> After we finished the above it's quite easy to move everything into
> dma_parms.  (At least if it's not a pointer, that then again needs an
> additional allocation.)

It should be a pointer. Adding dma stuff in device struct is not
correct logically because not all the users of device struct do dma.
We use device struct everywhere so making device struct fat is not a
good idea. I thought that we had the similar discussion when we
introduced dma_params.

So the above plan doesn't solve your problem. You still need an
additional allocation.


> > I'm not sure why this really troubles you. Can you give me a pointer
> > to what you have been working on? You have been working on non pci
> > device, right? Why can't you do like pci_dev, embedding
> > device_dma_parameters to your own device structure.
> I'm changing the way imx (ARCH=arm) registers its devices.  Currently
> we have in arch/arm/mach-imx/devices.c:
> 
> 	static u64 imx1_camera_dmamask = DMA_BIT_MASK(32);
> 
> 	struct platform_device imx1_camera_device = {
> 		...
> 		.dev = {
> 			.dma_mask = &imx1_camera_dmamask,
> 			.coherent_dma_mask = DMA_BIT_MASK(32),
> 		},
> 		...
> 	}

I know we have lots of such code especially in embedded archs but I
really don't like that. I think that dynamic registration is cleaner.


> and I want to make registration dynamic (e.g. using
> platform_device_register_resndata, see
> arch/arm/plat-mxc/devices/platform-imx-i2c.c for an example).
> 
> Currently I have a function imx_add_platform_device (that does the same
> as platform_device_register_resndata[1]) and now I want to register a
> device that needs .dma_mask set, so I added something like:
> 
> 	if (dmamask) {
>                 /*
>                  * This memory isn't freed when the device is put,
>                  * I don't have a nice idea for that though.  Conceptually
>                  * dma_mask in struct device should not be a pointer.
>                  * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
>                  */
>                 pdev->dev.dma_mask =
>                         kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
>                 if (!pdev->dev.dma_mask)
>                         goto err;
> 
>                 *pdev->dev.dma_mask = dmamask;
>                 pdev->dev.coherent_dma_mask = dmamask;
> 	}
> 
> So there is no private struct I could extend easily.  And I prefer
> cleaning up global oddities instead of being the x-th person to work
> around it.

As I explained above, your cleaning up plan doesn't work for you.

If you want to add dma_mask (and coherent) to device_dma_parameters
struct, I'm for that.

If you really want to avoid aditional allocation, you could invent
your device structure like pci_dev or add device_dma_parameters
struct to arm's pdev_archdata (I don't think that the latter is a good
idea).