| Message ID | 1431384089-28367-2-git-send-email-robh@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, May 11, 2015 at 05:41:26PM -0500, Rob Herring wrote: > Add the memory resource for the sa1100-rtc device. Since the memory > resource is already present in the pxa_rtc_resources, that makes > sa1100_rtc_resources and pxa_rtc_resources equivalent, so use > pxa_rtc_resources for both devices and remove the duplicate > sa1100_rtc_resources. This really isn't a good idea - what do you think happens when the same struct resource gets passed into insert_resource() twice?
On Mon, May 11, 2015 at 5:49 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Mon, May 11, 2015 at 05:41:26PM -0500, Rob Herring wrote: >> Add the memory resource for the sa1100-rtc device. Since the memory >> resource is already present in the pxa_rtc_resources, that makes >> sa1100_rtc_resources and pxa_rtc_resources equivalent, so use >> pxa_rtc_resources for both devices and remove the duplicate >> sa1100_rtc_resources. > > This really isn't a good idea - what do you think happens when > the same struct resource gets passed into insert_resource() > twice? Bad things. If you recall, we discussed this on v1[1]. See the commit msg of patch 2 for the summary. There is not currently a problem because the rtc-pxa driver does not request the resource. Rob [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/322355.html > > -- > FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up > according to speedtest.net.
Rob Herring <robh@kernel.org> writes: >> This really isn't a good idea - what do you think happens when >> the same struct resource gets passed into insert_resource() >> twice? > > Bad things. If you recall, we discussed this on v1[1]. See the commit > msg of patch 2 for the summary. There is not currently a problem > because the rtc-pxa driver does not request the resource. I think you're talking about resource claiming, while Russell is talking about resource declaration. The point Russell is raising is if you do a platform_add_device() twice with the same resource (which is done in pxa, once for sa1100-rtc, once for pxa-rtc), you call insert_resource() twice with the same resource structure. I had not thought of that before (that's in my reply noted "nasty consequences" in [1]), and I'll do my homework this week, and try this only patch, but if I follow Russell's thinking, then sa1100-rtc and pxa-rtc cannot be declared together anymore. Cheers. -- Robert
On Mon, May 11, 2015 at 07:35:24PM -0500, Rob Herring wrote: > On Mon, May 11, 2015 at 5:49 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > On Mon, May 11, 2015 at 05:41:26PM -0500, Rob Herring wrote: > >> Add the memory resource for the sa1100-rtc device. Since the memory > >> resource is already present in the pxa_rtc_resources, that makes > >> sa1100_rtc_resources and pxa_rtc_resources equivalent, so use > >> pxa_rtc_resources for both devices and remove the duplicate > >> sa1100_rtc_resources. > > > > This really isn't a good idea - what do you think happens when > > the same struct resource gets passed into insert_resource() > > twice? > > Bad things. If you recall, we discussed this on v1[1]. See the commit > msg of patch 2 for the summary. There is not currently a problem > because the rtc-pxa driver does not request the resource. There /is/ a problem, because it's not only the driver which does this, it's also the platform device code when the device is registered. See platform_device_add().
On Tuesday 12 May 2015 22:24:49 Robert Jarzmik wrote: > > I made the try, and Russell was right, this breaks the pxa architecture. The > relevant extract of kernel message is in [1]. > > The consequences with your patches : > - pxa27x_init() > - platform_add_devices() > - the pxa_device_rtc fails (as resource is duplicated) > - all the devices are unregistered (rollback) > And pxa27x fails. > > Now I'm pondering about the right approach : > - either remove sa1100_device_rtc from pxas > - or remove pxa_device_rtc > - or both > - or something else > > Let me think a bit about it. > To solve the problem with the duplicate registration of one resource, I'd suggest using platform_device_register_simple() for the registration, which will copy the resource. You can then mark the resource as __initconst and remove the device to save a little memory at runtime. Arnd
On Tue, May 12, 2015 at 10:30:12PM +0200, Arnd Bergmann wrote: > On Tuesday 12 May 2015 22:24:49 Robert Jarzmik wrote: > > > > I made the try, and Russell was right, this breaks the pxa architecture. The > > relevant extract of kernel message is in [1]. > > > > The consequences with your patches : > > - pxa27x_init() > > - platform_add_devices() > > - the pxa_device_rtc fails (as resource is duplicated) > > - all the devices are unregistered (rollback) > > And pxa27x fails. > > > > Now I'm pondering about the right approach : > > - either remove sa1100_device_rtc from pxas > > - or remove pxa_device_rtc > > - or both > > - or something else > > > > Let me think a bit about it. > > > > To solve the problem with the duplicate registration of one resource, > I'd suggest using platform_device_register_simple() for the registration, > which will copy the resource. You can then mark the resource as __initconst > and remove the device to save a little memory at runtime. No, a better solution is to solve the problem which requires the duplication in the first place, which is a broken driver structure.
On Tuesday 12 May 2015 21:36:13 Russell King - ARM Linux wrote: > On Tue, May 12, 2015 at 10:30:12PM +0200, Arnd Bergmann wrote: > > On Tuesday 12 May 2015 22:24:49 Robert Jarzmik wrote: > > > > > > I made the try, and Russell was right, this breaks the pxa architecture. The > > > relevant extract of kernel message is in [1]. > > > > > > The consequences with your patches : > > > - pxa27x_init() > > > - platform_add_devices() > > > - the pxa_device_rtc fails (as resource is duplicated) > > > - all the devices are unregistered (rollback) > > > And pxa27x fails. > > > > > > Now I'm pondering about the right approach : > > > - either remove sa1100_device_rtc from pxas > > > - or remove pxa_device_rtc > > > - or both > > > - or something else > > > > > > Let me think a bit about it. > > > > > > > To solve the problem with the duplicate registration of one resource, > > I'd suggest using platform_device_register_simple() for the registration, > > which will copy the resource. You can then mark the resource as __initconst > > and remove the device to save a little memory at runtime. > > No, a better solution is to solve the problem which requires the > duplication in the first place, which is a broken driver structure. Yes, makes sense. Or possibly do both ;-) Arnd
diff --git a/arch/arm/mach-pxa/devices.c b/arch/arm/mach-pxa/devices.c index 3543466..b598db6 100644 --- a/arch/arm/mach-pxa/devices.c +++ b/arch/arm/mach-pxa/devices.c @@ -439,25 +439,11 @@ struct platform_device pxa_device_rtc = { .resource = pxa_rtc_resources, }; -static struct resource sa1100_rtc_resources[] = { - { - .start = IRQ_RTC1Hz, - .end = IRQ_RTC1Hz, - .name = "rtc 1Hz", - .flags = IORESOURCE_IRQ, - }, { - .start = IRQ_RTCAlrm, - .end = IRQ_RTCAlrm, - .name = "rtc alarm", - .flags = IORESOURCE_IRQ, - }, -}; - struct platform_device sa1100_device_rtc = { .name = "sa1100-rtc", .id = -1, - .num_resources = ARRAY_SIZE(sa1100_rtc_resources), - .resource = sa1100_rtc_resources, + .num_resources = ARRAY_SIZE(pxa_rtc_resources), + .resource = pxa_rtc_resources, }; static struct resource pxa_ac97_resources[] = {