| Message ID | 20230310223027.315954-1-krzysztof.kozlowski@linaro.org |
|---|---|
| State | Accepted |
| Commit | 75f3d950054389e2556277e42170e37dd97cd872 |
| Headers | show |
| Series | [1/2] crypto - atmel-sha204a: Mark OF related data as maybe unused | expand |
On Fri, Mar 10, 2023 at 11:30:27PM +0100, Krzysztof Kozlowski wrote: > > diff --git a/drivers/crypto/img-hash.c b/drivers/crypto/img-hash.c > index fe93d19e3044..4e9a6660d791 100644 > --- a/drivers/crypto/img-hash.c > +++ b/drivers/crypto/img-hash.c > @@ -1106,7 +1106,7 @@ static struct platform_driver img_hash_driver = { > .driver = { > .name = "img-hash-accelerator", > .pm = &img_hash_pm_ops, > - .of_match_table = of_match_ptr(img_hash_match), > + .of_match_table = img_hash_match, I think we should keep this because this driver doesn't explicitly depend on OF. Sure of_match_table is unconditionally defined but I'd call that a bug instead of a feature :) However, I would take this if you resend it with a Kconfig update to add an explicit dependency on OF. Thanks,
On Fri, Mar 10, 2023 at 11:30:26PM +0100, Krzysztof Kozlowski wrote: > The driver can be compile tested with !CONFIG_OF making certain data > unused: > > drivers/crypto/atmel-sha204a.c:129:34: error: ‘atmel_sha204a_dt_ids’ defined but not used [-Werror=unused-const-variable=] > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > drivers/crypto/atmel-sha204a.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Patch applied. Thanks.
On 17/03/2023 04:04, Herbert Xu wrote: > On Fri, Mar 10, 2023 at 11:30:27PM +0100, Krzysztof Kozlowski wrote: >> >> diff --git a/drivers/crypto/img-hash.c b/drivers/crypto/img-hash.c >> index fe93d19e3044..4e9a6660d791 100644 >> --- a/drivers/crypto/img-hash.c >> +++ b/drivers/crypto/img-hash.c >> @@ -1106,7 +1106,7 @@ static struct platform_driver img_hash_driver = { >> .driver = { >> .name = "img-hash-accelerator", >> .pm = &img_hash_pm_ops, >> - .of_match_table = of_match_ptr(img_hash_match), >> + .of_match_table = img_hash_match, > > I think we should keep this because this driver doesn't explicitly > depend on OF. Sure of_match_table is unconditionally defined but > I'd call that a bug instead of a feature :) The missing dependency on OF is not a problem. The OF code is prepare and will work fine if the driver is built with !OF. The point is that with !OF after dropping of_match_ptr(), the driver could match via ACPI (PRP0001). If we make it depending on OF, the driver won't be able to use it, unless kernel is built with OF which is unlikely for ACPI systems. > > However, I would take this if you resend it with a Kconfig update > to add an explicit dependency on OF. > > Thanks, Best regards, Krzysztof
On Fri, Mar 17, 2023 at 09:12:05AM +0100, Krzysztof Kozlowski wrote: > > The missing dependency on OF is not a problem. The OF code is prepare > and will work fine if the driver is built with !OF. The point is that > with !OF after dropping of_match_ptr(), the driver could match via ACPI > (PRP0001). If we make it depending on OF, the driver won't be able to > use it, unless kernel is built with OF which is unlikely for ACPI systems. I know it works now, but what I'm saying is that if struct device_driver actually had of_match_table as conditional on OF, which ideally it should, then removing of_match_ptr will break the build. I know that it's currently unconditionally defined, but that's just wasting memory on non-OF machines such as x86. So either this driver is OF-only, in which case you can drop the of_match_ptr but must add a dependency on OF. Or it's not OF-only, in which case you should use of_match_ptr. Cheers,
On 17/03/2023 09:30, Herbert Xu wrote: > On Fri, Mar 17, 2023 at 09:12:05AM +0100, Krzysztof Kozlowski wrote: >> >> The missing dependency on OF is not a problem. The OF code is prepare >> and will work fine if the driver is built with !OF. The point is that >> with !OF after dropping of_match_ptr(), the driver could match via ACPI >> (PRP0001). If we make it depending on OF, the driver won't be able to >> use it, unless kernel is built with OF which is unlikely for ACPI systems. > > I know it works now, but what I'm saying is that if struct device_driver > actually had of_match_table as conditional on OF, which ideally it > should, then removing of_match_ptr will break the build. > > I know that it's currently unconditionally defined, but that's > just wasting memory on non-OF machines such as x86. That's not true. There is no waste because having it on x86 allows to match via ACPI PRP0001. It's on purpose there. > So either this driver is OF-only, in which case you can drop > the of_match_ptr but must add a dependency on OF. Or it's not > OF-only, in which case you should use of_match_ptr. There are OF-drivers used on ACPI and x86/arm64. The true question is whether this device will be ever used on ACPI via PRP0001, but you are not referring to this? Best regards, Krzysztof
On Fri, Mar 17, 2023 at 10:01:44AM +0100, Krzysztof Kozlowski wrote: > > That's not true. There is no waste because having it on x86 allows to > match via ACPI PRP0001. It's on purpose there. Alright how about this, I don't have any OF devices on my machine yet this structure is still taking up the extra memory for every single device driver. This is wrong. > There are OF-drivers used on ACPI and x86/arm64. Well then they should be selecting OF and everyone will be happy. Cheers,
On 17/03/2023 10:15, Herbert Xu wrote: > On Fri, Mar 17, 2023 at 10:01:44AM +0100, Krzysztof Kozlowski wrote: >> >> That's not true. There is no waste because having it on x86 allows to >> match via ACPI PRP0001. It's on purpose there. > > Alright how about this, I don't have any OF devices on my machine > yet this structure is still taking up the extra memory for every > single device driver. This is wrong. > >> There are OF-drivers used on ACPI and x86/arm64. > > Well then they should be selecting OF and everyone will be happy. OK, I will change it. It's a bit tiring to discuss the same concept with different maintainers and each time receive different point of view or each time need to convince that the other way is preferred. I already had such talks with Mark, so it is just easier change patch. Also, I tend to keep forgetting all the arguments. :) Let me just share also other maintainer's point of view: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0e62470652fa https://lore.kernel.org/all/20230311183534.1d0dfd64@jic23-huawei/ Anyway, I appreciate your feedback and thank you for picking up the first patch. I'll rework this one. Have a nice day! Best regards, Krzysztof
On Fri, 17 Mar 2023 at 10:16, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Fri, Mar 17, 2023 at 10:01:44AM +0100, Krzysztof Kozlowski wrote: > > > > That's not true. There is no waste because having it on x86 allows to > > match via ACPI PRP0001. It's on purpose there. > > Alright how about this, I don't have any OF devices on my machine > yet this structure is still taking up the extra memory for every > single device driver. This is wrong. > > > There are OF-drivers used on ACPI and x86/arm64. > > Well then they should be selecting OF and everyone will be happy. > No. PRP0001 support in ACPI does not depend on OF, so drivers that may be bound to such devices should not either. If you are concerned about the memory used by such tables, you can always propose making PRP0001 support configurable in the ACPI core, but making individual devices depend on OF for PRP0001 matching seems wrong to me.
diff --git a/drivers/crypto/atmel-sha204a.c b/drivers/crypto/atmel-sha204a.c index 4403dbb0f0b1..44a185a84760 100644 --- a/drivers/crypto/atmel-sha204a.c +++ b/drivers/crypto/atmel-sha204a.c @@ -126,7 +126,7 @@ static void atmel_sha204a_remove(struct i2c_client *client) kfree((void *)i2c_priv->hwrng.priv); } -static const struct of_device_id atmel_sha204a_dt_ids[] = { +static const struct of_device_id atmel_sha204a_dt_ids[] __maybe_unused = { { .compatible = "atmel,atsha204", }, { .compatible = "atmel,atsha204a", }, { /* sentinel */ }
The driver can be compile tested with !CONFIG_OF making certain data unused: drivers/crypto/atmel-sha204a.c:129:34: error: ‘atmel_sha204a_dt_ids’ defined but not used [-Werror=unused-const-variable=] Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- drivers/crypto/atmel-sha204a.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)