[PATCH] firmware: arm_scpi: Add hardware dependencies

[PATCH] firmware: arm_scpi: Add hardware dependencies

[Jean Delvare]

With a name like that, I assume that the ARM SCPI protocol is only
useful on the ARM architectures.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Fixes: 8f1498c03d15 ("firmware: arm_scpi: make it depend on MAILBOX instead of")
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Jon Medhurst (Tixy) <tixy@linaro.org>
---
Please correct me if I'm wrong.

 drivers/firmware/Kconfig |    1 +
 1 file changed, 1 insertion(+)

--- linux-4.10-rc5.orig/drivers/firmware/Kconfig	2017-01-01 23:31:53.000000000 +0100
+++ linux-4.10-rc5/drivers/firmware/Kconfig	2017-01-25 14:19:36.875930416 +0100
@@ -21,6 +21,7 @@ config ARM_PSCI_CHECKER
 
 config ARM_SCPI_PROTOCOL
 	tristate "ARM System Control and Power Interface (SCPI) Message Protocol"
+	depends on ARM || ARM64 || COMPILE_TEST
 	depends on MAILBOX
 	help
 	  System Control and Power Interface (SCPI) Message Protocol is


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] firmware: arm_scpi: Add hardware dependencies

[Sudeep Holla]

On 25/01/17 13:32, Jean Delvare wrote:
> With a name like that, I assume that the ARM SCPI protocol is only
> useful on the ARM architectures.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Fixes: 8f1498c03d15 ("firmware: arm_scpi: make it depend on MAILBOX instead of")
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Jon Medhurst (Tixy) <tixy@linaro.org>
> ---
> Please correct me if I'm wrong.
> 

I won't say you are wrong but the reason why it's named arm_scpi is
because the protocol was developed by ARM. It doesnn't mean only
ARM/ARM64 needs to use it, it can be used on any architecture for
inter-processor communication using any communication technique
(currently mailbox is the only supported in the driver)

-- 
Regards,
Sudeep

Re: [PATCH] firmware: arm_scpi: Add hardware dependencies

[Jean Delvare]

On Wed, 25 Jan 2017 13:38:47 +0000, Sudeep Holla wrote:
> On 25/01/17 13:32, Jean Delvare wrote:
> > With a name like that, I assume that the ARM SCPI protocol is only
> > useful on the ARM architectures.
> > 
> > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > Fixes: 8f1498c03d15 ("firmware: arm_scpi: make it depend on MAILBOX instead of")
> > Cc: Sudeep Holla <sudeep.holla@arm.com>
> > Cc: Jon Medhurst (Tixy) <tixy@linaro.org>
> > ---
> > Please correct me if I'm wrong.
> 
> I won't say you are wrong but the reason why it's named arm_scpi is
> because the protocol was developed by ARM. It doesnn't mean only
> ARM/ARM64 needs to use it, it can be used on any architecture for
> inter-processor communication using any communication technique
> (currently mailbox is the only supported in the driver)

OK, thanks for the clarification. In practice, what other architectures
are using it?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] firmware: arm_scpi: Add hardware dependencies

[Sudeep Holla]

On 25/01/17 13:50, Jean Delvare wrote:
> On Wed, 25 Jan 2017 13:38:47 +0000, Sudeep Holla wrote:
>> On 25/01/17 13:32, Jean Delvare wrote:
>>> With a name like that, I assume that the ARM SCPI protocol is only
>>> useful on the ARM architectures.
>>>
>>> Signed-off-by: Jean Delvare <jdelvare@suse.de>
>>> Fixes: 8f1498c03d15 ("firmware: arm_scpi: make it depend on MAILBOX instead of")
>>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>>> Cc: Jon Medhurst (Tixy) <tixy@linaro.org>
>>> ---
>>> Please correct me if I'm wrong.
>>
>> I won't say you are wrong but the reason why it's named arm_scpi is
>> because the protocol was developed by ARM. It doesnn't mean only
>> ARM/ARM64 needs to use it, it can be used on any architecture for
>> inter-processor communication using any communication technique
>> (currently mailbox is the only supported in the driver)
> 
> OK, thanks for the clarification. In practice, what other architectures
> are using it?
> 

None, hence I didn't say you are wrong ;). I am fine having the check if
it breaks for any other architecture with COMPILE_TEST.

Also you have mentioned it fixes 8f1498c03d15, have you seen any
regression with that commit ? If so, details in the commit would be good.
-- 
Regards,
Sudeep

Re: [PATCH] firmware: arm_scpi: Add hardware dependencies

[Jean Delvare]

On Wed, 25 Jan 2017 13:56:23 +0000, Sudeep Holla wrote:
> On 25/01/17 13:50, Jean Delvare wrote:
> > On Wed, 25 Jan 2017 13:38:47 +0000, Sudeep Holla wrote:
> >> On 25/01/17 13:32, Jean Delvare wrote:
> >>> With a name like that, I assume that the ARM SCPI protocol is only
> >>> useful on the ARM architectures.
> >>>
> >>> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> >>> Fixes: 8f1498c03d15 ("firmware: arm_scpi: make it depend on MAILBOX instead of")
> >>> Cc: Sudeep Holla <sudeep.holla@arm.com>
> >>> Cc: Jon Medhurst (Tixy) <tixy@linaro.org>
> >>> ---
> >>> Please correct me if I'm wrong.
> >>
> >> I won't say you are wrong but the reason why it's named arm_scpi is
> >> because the protocol was developed by ARM. It doesnn't mean only
> >> ARM/ARM64 needs to use it, it can be used on any architecture for
> >> inter-processor communication using any communication technique
> >> (currently mailbox is the only supported in the driver)
> > 
> > OK, thanks for the clarification. In practice, what other architectures
> > are using it?
> 
> None, hence I didn't say you are wrong ;). I am fine having the check if
> it breaks for any other architecture with COMPILE_TEST.

Not sure what you mean here... The purpose of COMPILE_TEST is to allow
limiting the scope of a driver withing hurting the build test coverage.

> Also you have mentioned it fixes 8f1498c03d15, have you seen any
> regression with that commit ? If so, details in the commit would be
> good.

Before 8f1498c03d15, the dependency on ARM_MHU made the driver only
visible on ARM kernels. Since 8f1498c03d15, the driver is proposed to
all, which I think isn't correct. In that sense my proposed patch is
fixing a (user-friendliness) regression. But nothing serious.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] firmware: arm_scpi: Add hardware dependencies

[Sudeep Holla]

On 25/01/17 14:14, Jean Delvare wrote:
> On Wed, 25 Jan 2017 13:56:23 +0000, Sudeep Holla wrote:
>> On 25/01/17 13:50, Jean Delvare wrote:
>>> On Wed, 25 Jan 2017 13:38:47 +0000, Sudeep Holla wrote:
>>>> On 25/01/17 13:32, Jean Delvare wrote:
>>>>> With a name like that, I assume that the ARM SCPI protocol is only
>>>>> useful on the ARM architectures.
>>>>>
>>>>> Signed-off-by: Jean Delvare <jdelvare@suse.de>
>>>>> Fixes: 8f1498c03d15 ("firmware: arm_scpi: make it depend on MAILBOX instead of")
>>>>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>>>>> Cc: Jon Medhurst (Tixy) <tixy@linaro.org>
>>>>> ---
>>>>> Please correct me if I'm wrong.
>>>>
>>>> I won't say you are wrong but the reason why it's named arm_scpi is
>>>> because the protocol was developed by ARM. It doesnn't mean only
>>>> ARM/ARM64 needs to use it, it can be used on any architecture for
>>>> inter-processor communication using any communication technique
>>>> (currently mailbox is the only supported in the driver)
>>>
>>> OK, thanks for the clarification. In practice, what other architectures
>>> are using it?
>>
>> None, hence I didn't say you are wrong ;). I am fine having the check if
>> it breaks for any other architecture with COMPILE_TEST.
> 
> Not sure what you mean here... The purpose of COMPILE_TEST is to allow
> limiting the scope of a driver withing hurting the build test coverage.
> 

No I agree with adding COMPILE_TEST just not ARM || ARM64

>> Also you have mentioned it fixes 8f1498c03d15, have you seen any
>> regression with that commit ? If so, details in the commit would be
>> good.
> 
> Before 8f1498c03d15, the dependency on ARM_MHU made the driver only
> visible on ARM kernels. Since 8f1498c03d15, the driver is proposed to
> all, which I think isn't correct. 

I disagree here. It depends on mailbox as we use mailbox API. And it is
now used on AmLogic Meson series of SoC. So it *is correct*.

> In that sense my proposed patch is
> fixing a (user-friendliness) regression. But nothing serious.
> 

Can you elaborate ? What's that *user-friendliness regression* ?
build/boot/... ? I just need more details.

-- 
Regards,
Sudeep

Re: [PATCH] firmware: arm_scpi: Add hardware dependencies

[Jean Delvare]

On Wed, 25 Jan 2017 14:20:53 +0000, Sudeep Holla wrote:
> On 25/01/17 14:14, Jean Delvare wrote:
> > On Wed, 25 Jan 2017 13:56:23 +0000, Sudeep Holla wrote:
> >> None, hence I didn't say you are wrong ;). I am fine having the check if
> >> it breaks for any other architecture with COMPILE_TEST.
> > 
> > Not sure what you mean here... The purpose of COMPILE_TEST is to allow
> > limiting the scope of a driver withing hurting the build test coverage.
> 
> No I agree with adding COMPILE_TEST just not ARM || ARM64

That doesn't make any sense. The only purpose of COMPILE_TEST is to be
or'd with an architecture or platform dependency. If you disagree with
adding an architecture or platform dependency, you can't agree with
adding COMPILE_TEST.

> >> Also you have mentioned it fixes 8f1498c03d15, have you seen any
> >> regression with that commit ? If so, details in the commit would be
> >> good.
> > 
> > Before 8f1498c03d15, the dependency on ARM_MHU made the driver only
> > visible on ARM kernels. Since 8f1498c03d15, the driver is proposed to
> > all, which I think isn't correct. 
> 
> I disagree here. It depends on mailbox as we use mailbox API. And it is
> now used on AmLogic Meson series of SoC. So it *is correct*.

I'm fine with depending on MAILBOX, I never proposed to change that.
But Meson are ARM SoCs. So what's the problem with making the driver
depend on ARM?

> > In that sense my proposed patch is
> > fixing a (user-friendliness) regression. But nothing serious.
> 
> Can you elaborate ? What's that *user-friendliness regression* ?
> build/boot/... ? I just need more details.

>From a distribution kernel maintainer point of view, having to answer
dozens of irrelevant questions with every kernel version update is
highly user-unfriendly. If all drivers could have proper hardware
dependencies from the start, it would be a lot easier.

And same holds for virtually everyone configuring his/her kernel. I'm
building a kernel for my x86 desktop PC, "make oldconfig" shouldn't
bother me with ARM drivers.

As a side note, there's no finger-pointing implied by Fixes: tags. They
are only meant to help people backporting patches, so that they know if
something they backported is later fixed up. It does not imply anything
regarding how serious the problem was.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] firmware: arm_scpi: Add hardware dependencies

[Sudeep Holla]

On 25/01/17 15:04, Jean Delvare wrote:
> On Wed, 25 Jan 2017 14:20:53 +0000, Sudeep Holla wrote:
>> On 25/01/17 14:14, Jean Delvare wrote:
>>> On Wed, 25 Jan 2017 13:56:23 +0000, Sudeep Holla wrote:
>>>> None, hence I didn't say you are wrong ;). I am fine having the check if
>>>> it breaks for any other architecture with COMPILE_TEST.
>>>
>>> Not sure what you mean here... The purpose of COMPILE_TEST is to allow
>>> limiting the scope of a driver withing hurting the build test coverage.
>>
>> No I agree with adding COMPILE_TEST just not ARM || ARM64
> 
> That doesn't make any sense. The only purpose of COMPILE_TEST is to be
> or'd with an architecture or platform dependency. If you disagree with
> adding an architecture or platform dependency, you can't agree with
> adding COMPILE_TEST.
> 

OK. As I said I am fine with that. I just wanted to know the details.
Also different maintainer have different opinion on how to use
COMPILE_TEST. I don't have any strong opinion on that.

>>>> Also you have mentioned it fixes 8f1498c03d15, have you seen any
>>>> regression with that commit ? If so, details in the commit would be
>>>> good.
>>>
>>> Before 8f1498c03d15, the dependency on ARM_MHU made the driver only
>>> visible on ARM kernels. Since 8f1498c03d15, the driver is proposed to
>>> all, which I think isn't correct. 
>>
>> I disagree here. It depends on mailbox as we use mailbox API. And it is
>> now used on AmLogic Meson series of SoC. So it *is correct*.
> 
> I'm fine with depending on MAILBOX, I never proposed to change that.
> But Meson are ARM SoCs. So what's the problem with making the driver
> depend on ARM?
> 

So I am not saying so, just fixes tag misled me. I assumed something was
broken with that change.

>>> In that sense my proposed patch is
>>> fixing a (user-friendliness) regression. But nothing serious.
>>
>> Can you elaborate ? What's that *user-friendliness regression* ?
>> build/boot/... ? I just need more details.
> 
> From a distribution kernel maintainer point of view, having to answer
> dozens of irrelevant questions with every kernel version update is
> highly user-unfriendly. If all drivers could have proper hardware
> dependencies from the start, it would be a lot easier.
> 

I agree, but generally I have seen suggestions no to add too much
dependency if it's not required and that's why I wanted to learn the
details.

> And same holds for virtually everyone configuring his/her kernel. I'm
> building a kernel for my x86 desktop PC, "make oldconfig" shouldn't
> bother me with ARM drivers.
> 

Agreed.

> As a side note, there's no finger-pointing implied by Fixes: tags. They
> are only meant to help people backporting patches, so that they know if
> something they backported is later fixed up. It does not imply anything
> regarding how serious the problem was.

Yes, but sometimes it's taken for stable tree and I don't think patch is
really fixing anything in that particular commit. e.g. what if x86 had
MAILBOX enabled, that patch changes nothing. So definitely not stable
material.

You can drop the fixes tag and add my ack. Please post it to
arm@kernel.org, so that ARM SoC team can pick this up directly.

-- 
Regards,
Sudeep

Re: [PATCH] firmware: arm_scpi: Add hardware dependencies

[Jean Delvare]

Hi Sudeep,

On Wed, 25 Jan 2017 15:15:45 +0000, Sudeep Holla wrote:
> Also different maintainer have different opinion on how to use
> COMPILE_TEST. I don't have any strong opinion on that.

There's not much to discuss or disagree about COMPILE_TEST. It was
introduced for one purpose and should be used for that one purpose:
restrict driver visibility to the relevant platforms without hindering
the build test coverage.

> (...)
> I agree, but generally I have seen suggestions no to add too much
> dependency if it's not required and that's why I wanted to learn the
> details.

I think you are speaking about something different. You don't want to
have dependencies on subsystems or options which your driver doesn't
actually need, of course. But limiting drivers to their intended or
actual architectures or platforms is a different kind of dependencies,
so that rule doesn't apply.

> > (...)
> > As a side note, there's no finger-pointing implied by Fixes: tags. They
> > are only meant to help people backporting patches, so that they know if
> > something they backported is later fixed up. It does not imply anything
> > regarding how serious the problem was.
> 
> Yes, but sometimes it's taken for stable tree

Really? I would expect that only patches tagged with "Cc:
stable@vger.kernel.org", or explicitly sent to that list, are
considered for stable trees. Greg, you don't pick commits for stable
trees just because they have a "Fixes:" tag, do you?

> and I don't think patch is
> really fixing anything in that particular commit. e.g. what if x86 had
> MAILBOX enabled, that patch changes nothing. So definitely not stable
> material.

It's not fixing a build breakage, if that's what you were worried
about. I agree it's not stable material and I did not Cc stable. For
me, Cc: stable@ and Fixes: are not correlated.

> You can drop the fixes tag and add my ack. Please post it to
> arm@kernel.org, so that ARM SoC team can pick this up directly.

I can't see this address in MAINTAINERS.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] firmware: arm_scpi: Add hardware dependencies

[Greg KH]

On Fri, Jan 27, 2017 at 09:53:17AM +0100, Jean Delvare wrote:
> On Wed, 25 Jan 2017 15:15:45 +0000, Sudeep Holla wrote:
> > > (...)
> > > As a side note, there's no finger-pointing implied by Fixes: tags. They
> > > are only meant to help people backporting patches, so that they know if
> > > something they backported is later fixed up. It does not imply anything
> > > regarding how serious the problem was.
> > 
> > Yes, but sometimes it's taken for stable tree
> 
> Really? I would expect that only patches tagged with "Cc:
> stable@vger.kernel.org", or explicitly sent to that list, are
> considered for stable trees. Greg, you don't pick commits for stable
> trees just because they have a "Fixes:" tag, do you?

Sometimes I do, usually because the maintainer has forgotten to put the
stable cc: (some subsystems are really bad about this), or because it
fixes a patch that was previous backported to stable.

I also take other patches that have no markings at all for stable trees,
depending on what they do, or who recommends them.  I try to scan all
commits for stuff that should be included that someone forgot to put a
stable@ cc: on.

But, just because you put a Fixes: tag, does not mean it will always go
into stable, because I do miss things at times.  If you know it should
go there, please always put the correct CC: tag.

thanks,

greg k-h

Re: [PATCH] firmware: arm_scpi: Add hardware dependencies

[Sudeep Holla]

On 27/01/17 08:53, Jean Delvare wrote:
> Hi Sudeep,
> 
> On Wed, 25 Jan 2017 15:15:45 +0000, Sudeep Holla wrote:

[...]

>> and I don't think patch is
>> really fixing anything in that particular commit. e.g. what if x86 had
>> MAILBOX enabled, that patch changes nothing. So definitely not stable
>> material.
> 
> It's not fixing a build breakage, if that's what you were worried
> about. I agree it's not stable material and I did not Cc stable. For
> me, Cc: stable@ and Fixes: are not correlated.
> 

I understand and I think Greg had replied which is much clear than what
I was trying to tell very vaguely.

>> You can drop the fixes tag and add my ack. Please post it to
>> arm@kernel.org, so that ARM SoC team can pick this up directly.
> 
> I can't see this address in MAINTAINERS.
> 

Ah yes, since I don't have any other patches I asked you to send it
directly to the email where ARM SoC picks up the patches most of the
time(the review still happens on the std. list, it's just one email
for the group of maintainer where the co-ordinate so that it's not missed.

If you stick my ack and post there along with the mailing list, I will
request them to pick it up directly instead of me sending pull request.

-- 
Regards,
Sudeep