| Message ID | 1484549107-5957-7-git-send-email-m.szyprowski@samsung.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jan 16, 2017 at 07:45:01AM +0100, Marek Szyprowski wrote: > Exynos5433 support has been added in parallel to adding initconst > annotation to most of the init data structures, so add those > annotations also to Exynos5433 structures. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/pinctrl/samsung/pinctrl-exynos.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c > index bf753a596209..70b94ad10cc1 100644 > --- a/drivers/pinctrl/samsung/pinctrl-exynos.c > +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c > @@ -1266,7 +1266,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata) > }; > > /* pin banks of exynos5433 pin-controller - ALIVE */ > -static const struct samsung_pin_bank_data exynos5433_pin_banks0[] = { > +static const struct samsung_pin_bank_data exynos5433_pin_banks0[] __initconst = { > EXYNOS5433_PIN_BANK_EINTW(8, 0x000, "gpa0", 0x00), > EXYNOS5433_PIN_BANK_EINTW(8, 0x020, "gpa1", 0x04), > EXYNOS5433_PIN_BANK_EINTW(8, 0x040, "gpa2", 0x08), > @@ -1279,28 +1279,28 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata) > }; Your change is aligned with existing code... but after I started to look into it I think this should not be initconst. The pin_banks (initconst) are referenced in pin_ctrl (initconst) which is referenced in samsung_pinctrl_dt_match (NOT initconst). The dt_match then is used in samsung_pinctrl_driver (for obvious reasons not initconst). We suppress the bind so this looks safe - this data should not be ever used after init - but it is not correct strictly speaking. Let's imagine some weird future platform which will use DT overlays with pinctrl. I think the overlays could affect the tree after the init stage. The other question is why the DEBUG_SECTION_MISMATCH is not complaining about this... Best regards, Krzysztof > > /* pin banks of exynos5433 pin-controller - AUD */ > -static const struct samsung_pin_bank_data exynos5433_pin_banks1[] = { > +static const struct samsung_pin_bank_data exynos5433_pin_banks1[] __initconst = { > EXYNOS5433_PIN_BANK_EINTG(7, 0x000, "gpz0", 0x00), > EXYNOS5433_PIN_BANK_EINTG(4, 0x020, "gpz1", 0x04), > }; > > /* pin banks of exynos5433 pin-controller - CPIF */ > -static const struct samsung_pin_bank_data exynos5433_pin_banks2[] = { > +static const struct samsung_pin_bank_data exynos5433_pin_banks2[] __initconst = { > EXYNOS5433_PIN_BANK_EINTG(2, 0x000, "gpv6", 0x00), > }; > > /* pin banks of exynos5433 pin-controller - eSE */ > -static const struct samsung_pin_bank_data exynos5433_pin_banks3[] = { > +static const struct samsung_pin_bank_data exynos5433_pin_banks3[] __initconst = { > EXYNOS5433_PIN_BANK_EINTG(3, 0x000, "gpj2", 0x00), > }; > > /* pin banks of exynos5433 pin-controller - FINGER */ > -static const struct samsung_pin_bank_data exynos5433_pin_banks4[] = { > +static const struct samsung_pin_bank_data exynos5433_pin_banks4[] __initconst = { > EXYNOS5433_PIN_BANK_EINTG(4, 0x000, "gpd5", 0x00), > }; > > /* pin banks of exynos5433 pin-controller - FSYS */ > -static const struct samsung_pin_bank_data exynos5433_pin_banks5[] = { > +static const struct samsung_pin_bank_data exynos5433_pin_banks5[] __initconst = { > EXYNOS5433_PIN_BANK_EINTG(6, 0x000, "gph1", 0x00), > EXYNOS5433_PIN_BANK_EINTG(7, 0x020, "gpr4", 0x04), > EXYNOS5433_PIN_BANK_EINTG(5, 0x040, "gpr0", 0x08), > @@ -1310,17 +1310,17 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata) > }; > > /* pin banks of exynos5433 pin-controller - IMEM */ > -static const struct samsung_pin_bank_data exynos5433_pin_banks6[] = { > +static const struct samsung_pin_bank_data exynos5433_pin_banks6[] __initconst = { > EXYNOS5433_PIN_BANK_EINTG(8, 0x000, "gpf0", 0x00), > }; > > /* pin banks of exynos5433 pin-controller - NFC */ > -static const struct samsung_pin_bank_data exynos5433_pin_banks7[] = { > +static const struct samsung_pin_bank_data exynos5433_pin_banks7[] __initconst = { > EXYNOS5433_PIN_BANK_EINTG(3, 0x000, "gpj0", 0x00), > }; > > /* pin banks of exynos5433 pin-controller - PERIC */ > -static const struct samsung_pin_bank_data exynos5433_pin_banks8[] = { > +static const struct samsung_pin_bank_data exynos5433_pin_banks8[] __initconst = { > EXYNOS5433_PIN_BANK_EINTG(6, 0x000, "gpv7", 0x00), > EXYNOS5433_PIN_BANK_EINTG(5, 0x020, "gpb0", 0x04), > EXYNOS5433_PIN_BANK_EINTG(8, 0x040, "gpc0", 0x08), > @@ -1341,7 +1341,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata) > }; > > /* pin banks of exynos5433 pin-controller - TOUCH */ > -static const struct samsung_pin_bank_data exynos5433_pin_banks9[] = { > +static const struct samsung_pin_bank_data exynos5433_pin_banks9[] __initconst = { > EXYNOS5433_PIN_BANK_EINTG(3, 0x000, "gpj1", 0x00), > }; > > @@ -1349,7 +1349,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata) > * Samsung pinctrl driver data for Exynos5433 SoC. Exynos5433 SoC includes > * ten gpio/pin-mux/pinconfig controllers. > */ > -const struct samsung_pin_ctrl exynos5433_pin_ctrl[] = { > +const struct samsung_pin_ctrl exynos5433_pin_ctrl[] __initconst = { > { > /* pin-controller instance 0 data */ > .pin_banks = exynos5433_pin_banks0, > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2017-01-17 4:14 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>: > On Mon, Jan 16, 2017 at 07:45:01AM +0100, Marek Szyprowski wrote: >> Exynos5433 support has been added in parallel to adding initconst >> annotation to most of the init data structures, so add those >> annotations also to Exynos5433 structures. >> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> drivers/pinctrl/samsung/pinctrl-exynos.c | 22 +++++++++++----------- >> 1 file changed, 11 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c >> index bf753a596209..70b94ad10cc1 100644 >> --- a/drivers/pinctrl/samsung/pinctrl-exynos.c >> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c >> @@ -1266,7 +1266,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata) >> }; >> >> /* pin banks of exynos5433 pin-controller - ALIVE */ >> -static const struct samsung_pin_bank_data exynos5433_pin_banks0[] = { >> +static const struct samsung_pin_bank_data exynos5433_pin_banks0[] __initconst = { >> EXYNOS5433_PIN_BANK_EINTW(8, 0x000, "gpa0", 0x00), >> EXYNOS5433_PIN_BANK_EINTW(8, 0x020, "gpa1", 0x04), >> EXYNOS5433_PIN_BANK_EINTW(8, 0x040, "gpa2", 0x08), >> @@ -1279,28 +1279,28 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata) >> }; > > Your change is aligned with existing code... but after I started to look > into it I think this should not be initconst. > > The pin_banks (initconst) are referenced in pin_ctrl (initconst) which > is referenced in samsung_pinctrl_dt_match (NOT initconst). The dt_match > then is used in samsung_pinctrl_driver (for obvious reasons not > initconst). > > We suppress the bind so this looks safe - this data should not be ever used > after init - but it is not correct strictly speaking. > > Let's imagine some weird future platform which will use DT overlays with > pinctrl. I think the overlays could affect the tree after the init > stage. I think it's not very realistic to have a Samsung on-SoC pin controller in an overlay. AFAIR we already assume in several places that this driver fully initializes in init stage, so we can save some memory by discarding this data Still, I guess we could add some measure to make sure nothing attempts to probe this driver after the data is discarded. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 17, 2017 at 6:44 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > 2017-01-17 4:14 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>: >> On Mon, Jan 16, 2017 at 07:45:01AM +0100, Marek Szyprowski wrote: >>> Exynos5433 support has been added in parallel to adding initconst >>> annotation to most of the init data structures, so add those >>> annotations also to Exynos5433 structures. >>> >>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>> --- >>> drivers/pinctrl/samsung/pinctrl-exynos.c | 22 +++++++++++----------- >>> 1 file changed, 11 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c >>> index bf753a596209..70b94ad10cc1 100644 >>> --- a/drivers/pinctrl/samsung/pinctrl-exynos.c >>> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c >>> @@ -1266,7 +1266,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata) >>> }; >>> >>> /* pin banks of exynos5433 pin-controller - ALIVE */ >>> -static const struct samsung_pin_bank_data exynos5433_pin_banks0[] = { >>> +static const struct samsung_pin_bank_data exynos5433_pin_banks0[] __initconst = { >>> EXYNOS5433_PIN_BANK_EINTW(8, 0x000, "gpa0", 0x00), >>> EXYNOS5433_PIN_BANK_EINTW(8, 0x020, "gpa1", 0x04), >>> EXYNOS5433_PIN_BANK_EINTW(8, 0x040, "gpa2", 0x08), >>> @@ -1279,28 +1279,28 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata) >>> }; >> >> Your change is aligned with existing code... but after I started to look >> into it I think this should not be initconst. >> >> The pin_banks (initconst) are referenced in pin_ctrl (initconst) which >> is referenced in samsung_pinctrl_dt_match (NOT initconst). The dt_match >> then is used in samsung_pinctrl_driver (for obvious reasons not >> initconst). >> >> We suppress the bind so this looks safe - this data should not be ever used >> after init - but it is not correct strictly speaking. >> >> Let's imagine some weird future platform which will use DT overlays with >> pinctrl. I think the overlays could affect the tree after the init >> stage. > > I think it's not very realistic to have a Samsung on-SoC pin > controller in an overlay. AFAIR we already assume in several places > that this driver fully initializes in init stage, so we can save some > memory by discarding this data Two things here. Either we write proper code or we write code for "believe it will work". There is a problem with second approach - after some time, the driver will be developed further it your assumptions might change... then stuff might get broken because no one expected that driver data is discarded. Code working only with some assumptions is a more difficult to maintain than code which is correct. In the same time if you believe your code is "correct" then just mark samsung_pinctrl_dt_match initconst... Marking only few parts in the chain is broken. Second thing. How much memory you are saving? 5 kB in total? 10 kB? Is it worth the risk? > Still, I guess we could add some measure to make sure nothing attempts > to probe this driver after the data is discarded. That could work... but last time I asked for checking the return value of match_data then you (I think) and Bartlomiej responded "no sense". So please be consistent. Anyway it is not possible to NULL-ify the match_data after first probe because samsung_pinctrl_dt_match is const. This means you would have to add some crazy logic to check for late (from overlays) or second probe and discard it... just because we wanted to save some memory and marked initconst something referenced from non-initconst section. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2017-01-17 15:34 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>: > On Tue, Jan 17, 2017 at 6:44 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: >> 2017-01-17 4:14 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>: >>> On Mon, Jan 16, 2017 at 07:45:01AM +0100, Marek Szyprowski wrote: >>>> Exynos5433 support has been added in parallel to adding initconst >>>> annotation to most of the init data structures, so add those >>>> annotations also to Exynos5433 structures. >>>> >>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>>> --- >>>> drivers/pinctrl/samsung/pinctrl-exynos.c | 22 +++++++++++----------- >>>> 1 file changed, 11 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c >>>> index bf753a596209..70b94ad10cc1 100644 >>>> --- a/drivers/pinctrl/samsung/pinctrl-exynos.c >>>> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c >>>> @@ -1266,7 +1266,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata) >>>> }; >>>> >>>> /* pin banks of exynos5433 pin-controller - ALIVE */ >>>> -static const struct samsung_pin_bank_data exynos5433_pin_banks0[] = { >>>> +static const struct samsung_pin_bank_data exynos5433_pin_banks0[] __initconst = { >>>> EXYNOS5433_PIN_BANK_EINTW(8, 0x000, "gpa0", 0x00), >>>> EXYNOS5433_PIN_BANK_EINTW(8, 0x020, "gpa1", 0x04), >>>> EXYNOS5433_PIN_BANK_EINTW(8, 0x040, "gpa2", 0x08), >>>> @@ -1279,28 +1279,28 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata) >>>> }; >>> >>> Your change is aligned with existing code... but after I started to look >>> into it I think this should not be initconst. >>> >>> The pin_banks (initconst) are referenced in pin_ctrl (initconst) which >>> is referenced in samsung_pinctrl_dt_match (NOT initconst). The dt_match >>> then is used in samsung_pinctrl_driver (for obvious reasons not >>> initconst). >>> >>> We suppress the bind so this looks safe - this data should not be ever used >>> after init - but it is not correct strictly speaking. >>> >>> Let's imagine some weird future platform which will use DT overlays with >>> pinctrl. I think the overlays could affect the tree after the init >>> stage. >> >> I think it's not very realistic to have a Samsung on-SoC pin >> controller in an overlay. AFAIR we already assume in several places >> that this driver fully initializes in init stage, so we can save some >> memory by discarding this data > > Two things here. Either we write proper code or we write code for > "believe it will work". There is a problem with second approach - > after some time, the driver will be developed further it your > assumptions might change... then stuff might get broken because no one > expected that driver data is discarded. Code working only with some > assumptions is a more difficult to maintain than code which is > correct. We write code that is proper for the assumptions we made during the design, i.e. design decisions. Otherwise you would end up with the driver bloated everywhere with handing things that don't make any sense, which would be definitely _not_ easier to maintain. Of course we can have some trivial code to enforce the assumptions, as I mentioned before. This is a driver for an on-chip low level device that is very, very unlikely to be hotpluggable and almost any other driver for remaining on-chip devices depend on it. It doesn't mean that we can't change the design decision later, if in future it becomes feasible to load low-level SoC drivers from modules then we can make the driver support so. > > In the same time if you believe your code is "correct" then just mark > samsung_pinctrl_dt_match initconst... Marking only few parts in the > chain is broken. I haven't said that samsung_pinctrl_dt_match should be kept non-initconst. It's indeed a good idea to make it initconst as well. > > Second thing. How much memory you are saving? 5 kB in total? 10 kB? Is > it worth the risk? 10 kB in one driver. Multiply by the number of similar low level drivers for all SoCs supported by ARM v7 multiplatform build and you can get quite a significant number of memory saved... > >> Still, I guess we could add some measure to make sure nothing attempts >> to probe this driver after the data is discarded. > > That could work... but last time I asked for checking the return value > of match_data then you (I think) and Bartlomiej responded "no sense". > So please be consistent. That's because this is already enforced by the design of the driver model... > Anyway it is not possible to NULL-ify the > match_data after first probe because samsung_pinctrl_dt_match is > const. This means you would have to add some crazy logic to check for > late (from overlays) or second probe and discard it... just because we > wanted to save some memory and marked initconst something referenced > from non-initconst section. ...and similarly the assumptions we are talking about in case of this problem could be enforced the same way. There is already platform_driver_probe() that is supposed to be used for such on-SoC non-hotpluggable devices and which triggers a one-time probe. Unfortunately it is a bit broken currently, because it can't handle deferred probes. Anyway, I'm not even insisting that we should keep this initconst annotation. Just wanted to point out that there are valid reasons for it to be useful and correct (in terms of the design decisions taken when writing the driver). Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c index bf753a596209..70b94ad10cc1 100644 --- a/drivers/pinctrl/samsung/pinctrl-exynos.c +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c @@ -1266,7 +1266,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata) }; /* pin banks of exynos5433 pin-controller - ALIVE */ -static const struct samsung_pin_bank_data exynos5433_pin_banks0[] = { +static const struct samsung_pin_bank_data exynos5433_pin_banks0[] __initconst = { EXYNOS5433_PIN_BANK_EINTW(8, 0x000, "gpa0", 0x00), EXYNOS5433_PIN_BANK_EINTW(8, 0x020, "gpa1", 0x04), EXYNOS5433_PIN_BANK_EINTW(8, 0x040, "gpa2", 0x08), @@ -1279,28 +1279,28 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata) }; /* pin banks of exynos5433 pin-controller - AUD */ -static const struct samsung_pin_bank_data exynos5433_pin_banks1[] = { +static const struct samsung_pin_bank_data exynos5433_pin_banks1[] __initconst = { EXYNOS5433_PIN_BANK_EINTG(7, 0x000, "gpz0", 0x00), EXYNOS5433_PIN_BANK_EINTG(4, 0x020, "gpz1", 0x04), }; /* pin banks of exynos5433 pin-controller - CPIF */ -static const struct samsung_pin_bank_data exynos5433_pin_banks2[] = { +static const struct samsung_pin_bank_data exynos5433_pin_banks2[] __initconst = { EXYNOS5433_PIN_BANK_EINTG(2, 0x000, "gpv6", 0x00), }; /* pin banks of exynos5433 pin-controller - eSE */ -static const struct samsung_pin_bank_data exynos5433_pin_banks3[] = { +static const struct samsung_pin_bank_data exynos5433_pin_banks3[] __initconst = { EXYNOS5433_PIN_BANK_EINTG(3, 0x000, "gpj2", 0x00), }; /* pin banks of exynos5433 pin-controller - FINGER */ -static const struct samsung_pin_bank_data exynos5433_pin_banks4[] = { +static const struct samsung_pin_bank_data exynos5433_pin_banks4[] __initconst = { EXYNOS5433_PIN_BANK_EINTG(4, 0x000, "gpd5", 0x00), }; /* pin banks of exynos5433 pin-controller - FSYS */ -static const struct samsung_pin_bank_data exynos5433_pin_banks5[] = { +static const struct samsung_pin_bank_data exynos5433_pin_banks5[] __initconst = { EXYNOS5433_PIN_BANK_EINTG(6, 0x000, "gph1", 0x00), EXYNOS5433_PIN_BANK_EINTG(7, 0x020, "gpr4", 0x04), EXYNOS5433_PIN_BANK_EINTG(5, 0x040, "gpr0", 0x08), @@ -1310,17 +1310,17 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata) }; /* pin banks of exynos5433 pin-controller - IMEM */ -static const struct samsung_pin_bank_data exynos5433_pin_banks6[] = { +static const struct samsung_pin_bank_data exynos5433_pin_banks6[] __initconst = { EXYNOS5433_PIN_BANK_EINTG(8, 0x000, "gpf0", 0x00), }; /* pin banks of exynos5433 pin-controller - NFC */ -static const struct samsung_pin_bank_data exynos5433_pin_banks7[] = { +static const struct samsung_pin_bank_data exynos5433_pin_banks7[] __initconst = { EXYNOS5433_PIN_BANK_EINTG(3, 0x000, "gpj0", 0x00), }; /* pin banks of exynos5433 pin-controller - PERIC */ -static const struct samsung_pin_bank_data exynos5433_pin_banks8[] = { +static const struct samsung_pin_bank_data exynos5433_pin_banks8[] __initconst = { EXYNOS5433_PIN_BANK_EINTG(6, 0x000, "gpv7", 0x00), EXYNOS5433_PIN_BANK_EINTG(5, 0x020, "gpb0", 0x04), EXYNOS5433_PIN_BANK_EINTG(8, 0x040, "gpc0", 0x08), @@ -1341,7 +1341,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata) }; /* pin banks of exynos5433 pin-controller - TOUCH */ -static const struct samsung_pin_bank_data exynos5433_pin_banks9[] = { +static const struct samsung_pin_bank_data exynos5433_pin_banks9[] __initconst = { EXYNOS5433_PIN_BANK_EINTG(3, 0x000, "gpj1", 0x00), }; @@ -1349,7 +1349,7 @@ static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata) * Samsung pinctrl driver data for Exynos5433 SoC. Exynos5433 SoC includes * ten gpio/pin-mux/pinconfig controllers. */ -const struct samsung_pin_ctrl exynos5433_pin_ctrl[] = { +const struct samsung_pin_ctrl exynos5433_pin_ctrl[] __initconst = { { /* pin-controller instance 0 data */ .pin_banks = exynos5433_pin_banks0,
Exynos5433 support has been added in parallel to adding initconst annotation to most of the init data structures, so add those annotations also to Exynos5433 structures. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/pinctrl/samsung/pinctrl-exynos.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)