Odd dependency on ARM_SCMI_PROTOCOL

Odd dependency on ARM_SCMI_PROTOCOL

[Jean Delvare]

Hi all,

I noticed that the following dependency construct is used several times
throughout the kernel tree:

depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)

Maybe I'm just missing something as I'm not familiar with SCMI, but I
can't really make sense of this. Considering that ARM_SCMI_PROTOCOL
does not depend on OF, I do not understand why it would be OK to build
these drivers under ARM_SCMI_PROTOCOL without OF, but if test-building
under COMPILE_TEST instead, then OF would suddenly be needed. Can
anyone explain the logic?

Considering that ARM_SCMI_PROTOCOL can be enabled through COMPILE_TEST,
and OF is now available on architectures, I think this dependency is
needlessly complex and these drivers could simply use:

depends on ARM_SCMI_PROTOCOL

What do you think?

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: Odd dependency on ARM_SCMI_PROTOCOL

[Cristian Marussi]

On Tue, Oct 22, 2024 at 01:55:37PM +0200, Jean Delvare wrote:
> Hi all,
> 

Hi,

this pre-dates me, so take this with a grain of salt, and Sudeep may
have a better explanation...

> I noticed that the following dependency construct is used several times
> throughout the kernel tree:
> 
> depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> 
> Maybe I'm just missing something as I'm not familiar with SCMI, but I
> can't really make sense of this. Considering that ARM_SCMI_PROTOCOL
> does not depend on OF, I do not understand why it would be OK to build
> these drivers under ARM_SCMI_PROTOCOL without OF, but if test-building
> under COMPILE_TEST instead, then OF would suddenly be needed. Can
> anyone explain the logic?
> 
> Considering that ARM_SCMI_PROTOCOL can be enabled through COMPILE_TEST,
> and OF is now available on architectures, I think this dependency is
> needlessly complex and these drivers could simply use:

... my guess is that ARM_SCMI_PROTOCOL does NOT explicitly depends on OF
simply because it depends on ARM|ARM64 which have an implicit OF dependency...
....indeed in order to use the SCMI stack you need OF...

...not saying that this is right....just trying to interpret here...
...on the other side for these reasons, if you dont have OF implicitly from
the arch, you cannot COMPILE_TEST the SCMI stack without OF, so the
complex dependency

Probably this should be done better in Kconfig...but lets see if Sudeep
can correct me...

Thanks,
Cristian

Re: Odd dependency on ARM_SCMI_PROTOCOL

[Sudeep Holla]

On Tue, Oct 22, 2024 at 01:22:13PM +0100, Cristian Marussi wrote:
> On Tue, Oct 22, 2024 at 01:55:37PM +0200, Jean Delvare wrote:
> > Hi all,
> >
>
> Hi,
>
> this pre-dates me, so take this with a grain of salt, and Sudeep may
> have a better explanation...

Indeed, I had to dig a bit and found a patch from 2016.

> > I noticed that the following dependency construct is used several times
> > throughout the kernel tree:
> >
> > depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> >
> > Maybe I'm just missing something as I'm not familiar with SCMI, but I
> > can't really make sense of this. Considering that ARM_SCMI_PROTOCOL
> > does not depend on OF, I do not understand why it would be OK to build
> > these drivers under ARM_SCMI_PROTOCOL without OF, but if test-building
> > under COMPILE_TEST instead, then OF would suddenly be needed. Can
> > anyone explain the logic?
> >
> > Considering that ARM_SCMI_PROTOCOL can be enabled through COMPILE_TEST,
> > and OF is now available on architectures, I think this dependency is
> > needlessly complex and these drivers could simply use:
>
> ... my guess is that ARM_SCMI_PROTOCOL does NOT explicitly depends on OF
> simply because it depends on ARM|ARM64 which have an implicit OF dependency...
> ....indeed in order to use the SCMI stack you need OF...
>
> ...not saying that this is right....just trying to interpret here...
> ...on the other side for these reasons, if you dont have OF implicitly from
> the arch, you cannot COMPILE_TEST the SCMI stack without OF, so the
> complex dependency
>
> Probably this should be done better in Kconfig...but lets see if Sudeep
> can correct me...
>

Commit e517dfe67480 ("firmware: scpi: add CONFIG_OF dependency") is the
one that I copied to SCMI as well as we wanted  COMPILE_TEST coverage on
SCMI as well. As per the commit, !OF will give some warnings for COMPILE_TEST
which the above commit addresses. It is just copy of that you see in SCMI
as SCMI replaced the old SCPI and is used more widely and had more framework
support, hence you will find it at more places than SCPI.

Hope that explains, happy to take a deeper look if you have or find issues
with the way it exists today.

--
Regards,
Sudeep

Re: Odd dependency on ARM_SCMI_PROTOCOL

[Jean Delvare]

Hi Sudeep and Christian,

On Tue, 22 Oct 2024 16:01:28 +0100, Sudeep Holla wrote:
> On Tue, Oct 22, 2024 at 01:22:13PM +0100, Cristian Marussi wrote:
> > On Tue, Oct 22, 2024 at 01:55:37PM +0200, Jean Delvare wrote:  
> > > Hi all,
> >
> > this pre-dates me, so take this with a grain of salt, and Sudeep may
> > have a better explanation...  
> 
> Indeed, I had to dig a bit and found a patch from 2016.
> 
> > > I noticed that the following dependency construct is used several times
> > > throughout the kernel tree:
> > >
> > > depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> > >
> > > Maybe I'm just missing something as I'm not familiar with SCMI, but I
> > > can't really make sense of this. Considering that ARM_SCMI_PROTOCOL
> > > does not depend on OF, I do not understand why it would be OK to build
> > > these drivers under ARM_SCMI_PROTOCOL without OF, but if test-building
> > > under COMPILE_TEST instead, then OF would suddenly be needed. Can
> > > anyone explain the logic?
> > >
> > > Considering that ARM_SCMI_PROTOCOL can be enabled through COMPILE_TEST,
> > > and OF is now available on architectures, I think this dependency is
> > > needlessly complex and these drivers could simply use:  
> >
> > ... my guess is that ARM_SCMI_PROTOCOL does NOT explicitly depends on OF
> > simply because it depends on ARM|ARM64 which have an implicit OF dependency...
> > ....indeed in order to use the SCMI stack you need OF...
> >
> > ...not saying that this is right....just trying to interpret here...
> > ...on the other side for these reasons, if you dont have OF implicitly from
> > the arch, you cannot COMPILE_TEST the SCMI stack without OF, so the
> > complex dependency
> >
> > Probably this should be done better in Kconfig...but lets see if Sudeep
> > can correct me...
> 
> Commit e517dfe67480 ("firmware: scpi: add CONFIG_OF dependency") is the
> one that I copied to SCMI as well as we wanted  COMPILE_TEST coverage on
> SCMI as well. As per the commit, !OF will give some warnings for COMPILE_TEST
> which the above commit addresses. It is just copy of that you see in SCMI
> as SCMI replaced the old SCPI and is used more widely and had more framework
> support, hence you will find it at more places than SCPI.
> 
> Hope that explains, happy to take a deeper look if you have or find issues
> with the way it exists today.

Thanks for the explanations. It makes sense now. That being said, I
believe that the same dependencies would be better expressed as:

depends on OF
depends on ARM_SCMI_PROTOCOL || COMPILE_TEST

because 1* the code technically needs OF to build, and 2* these
drivers are only useful on kernels which have ARM_SCMI_PROTOCOL, unless
build-testing. The result would be the same as what we have now, but
the intentions would be clearer and the correctness more obvious IMHO.

-- 
Jean Delvare
SUSE L3 Support

Re: Odd dependency on ARM_SCMI_PROTOCOL

[Sudeep Holla]

On Tue, Oct 22, 2024 at 06:13:49PM +0200, Jean Delvare wrote:
> Hi Sudeep and Christian,
> 
> On Tue, 22 Oct 2024 16:01:28 +0100, Sudeep Holla wrote:
> > On Tue, Oct 22, 2024 at 01:22:13PM +0100, Cristian Marussi wrote:
> > > On Tue, Oct 22, 2024 at 01:55:37PM +0200, Jean Delvare wrote:  
> > > > Hi all,
> > >
> > > this pre-dates me, so take this with a grain of salt, and Sudeep may
> > > have a better explanation...  
> > 
> > Indeed, I had to dig a bit and found a patch from 2016.
> > 
> > > > I noticed that the following dependency construct is used several times
> > > > throughout the kernel tree:
> > > >
> > > > depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> > > >
> > > > Maybe I'm just missing something as I'm not familiar with SCMI, but I
> > > > can't really make sense of this. Considering that ARM_SCMI_PROTOCOL
> > > > does not depend on OF, I do not understand why it would be OK to build
> > > > these drivers under ARM_SCMI_PROTOCOL without OF, but if test-building
> > > > under COMPILE_TEST instead, then OF would suddenly be needed. Can
> > > > anyone explain the logic?
> > > >
> > > > Considering that ARM_SCMI_PROTOCOL can be enabled through COMPILE_TEST,
> > > > and OF is now available on architectures, I think this dependency is
> > > > needlessly complex and these drivers could simply use:  
> > >
> > > ... my guess is that ARM_SCMI_PROTOCOL does NOT explicitly depends on OF
> > > simply because it depends on ARM|ARM64 which have an implicit OF dependency...
> > > ....indeed in order to use the SCMI stack you need OF...
> > >
> > > ...not saying that this is right....just trying to interpret here...
> > > ...on the other side for these reasons, if you dont have OF implicitly from
> > > the arch, you cannot COMPILE_TEST the SCMI stack without OF, so the
> > > complex dependency
> > >
> > > Probably this should be done better in Kconfig...but lets see if Sudeep
> > > can correct me...
> > 
> > Commit e517dfe67480 ("firmware: scpi: add CONFIG_OF dependency") is the
> > one that I copied to SCMI as well as we wanted  COMPILE_TEST coverage on
> > SCMI as well. As per the commit, !OF will give some warnings for COMPILE_TEST
> > which the above commit addresses. It is just copy of that you see in SCMI
> > as SCMI replaced the old SCPI and is used more widely and had more framework
> > support, hence you will find it at more places than SCPI.
> > 
> > Hope that explains, happy to take a deeper look if you have or find issues
> > with the way it exists today.
> 
> Thanks for the explanations. It makes sense now. That being said, I
> believe that the same dependencies would be better expressed as:
> 
> depends on OF
> depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
> 
> because 1* the code technically needs OF to build,

Not true, just force enabled SCMI on x86 without COMPILE_TEST and COMPILE_OF
and it builds just fine. But yes, I removed dependency to enabled it on x86

> and 2* these drivers are only useful on kernels which have ARM_SCMI_PROTOCOL,
> unless build-testing. The result would be the same as what we have now, but
> the intentions would be clearer and the correctness more obvious IMHO.
>

I am just trying to understand why it is done so, but can't think of any
reason that contradicts you statement above. The code itself can cope up
with CONFIG_OF not enabled, so unless I can trigger the warning that Arnd
faced when he pushed the initial patch on SCPI, I don't have much useful to
add. You can post a patch cc-ing him and take up the discussion may be. I
don't have a strong opinion either way.

-- 
Regards,
Sudeep