[2/2] crypto - img-hash: Drop of_match_ptr for ID table

  The driver can match only via the DT table so the table should be always
used and the of_match_ptr does not have any sense (this also allows ACPI
matching via PRP0001, even though it is not relevant here).

  drivers/crypto/img-hash.c:930:34: error: ‘img_hash_match’ defined but not used [-Werror=unused-const-variable=]

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/crypto/img-hash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

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 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.
  

On Thu, Mar 23, 2023 at 09:43:58AM +0100, Ard Biesheuvel wrote:
>
> 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.

What I am against is removing of_match_ptr by doing a million
tiny patches.  Either we should keep it, in which case the
of_match_table field should be made conditional on OF or perhaps
a new Kconfig option if there is no way to reconcile this with
ACPI, or we decide to get rid of it and you should do one giant
patch to remove of_match_ptr across the kernel tree.

We should not be doing a patch for every single driver that has
of_match_table based on whether it can or cannot be used through
ACPI.

Cheers,