Re: [PATCH] comedi: Fix driver module dependencies since HAS_IOPORT changes

[Arnd Bergmann]

On Mon, Sep 4, 2023, at 06:10, Ian Abbott wrote:
> On 03/09/2023 16:49, Arnd Bergmann wrote:
>> On Fri, Sep 1, 2023, at 15:26, Ian Abbott wrote:
>>> Commit b5c75b68b7de ("comedi: add HAS_IOPORT dependencies") changed the
>>> "select" directives to "depend on" directives for several config
>>> stanzas, but the options they depended on could not be selected,
>>> breaking previously selected options.
>> 
>> Right, I think that correctly describes the regression, sorry I didn't
>> catch that during the submission.
>> 
>>>   Change them back to "select"
>>> directives and add "depends on HAS_IOPORT" to config entries for modules
>>> that either use inb()/outb() and friends directly, or (recursively)
>>> depend on modules that do so.
>> 
>> This also describes a correct solution to the problem, but from looking
>> at your patch, I think it's not exactly what you do.
>> 
>>>
>>>   config COMEDI_PCL711
>>>   	tristate "Advantech PCL-711/711b and ADlink ACL-8112 ISA card support"
>>> -	depends on HAS_IOPORT
>>> -	depends on COMEDI_8254
>>> +	select COMEDI_8254
>> 
>> If COMEDI_8254 depends on HAS_IOPORT, you must not drop the 'depends on'
>> here, otherwise you get build failures from missing dependencies.
>> 
>> Same thing for a lot of the ones below. You should only change the
>> select, but not remove the 'depends on HAS_IOPORT' in any of these,
>> unless the entire Kconfig file already has this.
>
> I assumed it was OK because it is only selectable if 'ISA' is selected 
> and all the other ISA card drivers that use inb()/outb() and friends do 
> not depend on 'HAS_IOPORT' either.

Ok, that is probably true if there s an implied 'depends on ISA', but
I wouldn't change the logic as part of a bug fix, since the 'depends on
HAS_IOPORT' is not wrong.

>> This change looks unrelated to both your description and
>> the bug, as you are just moving around the dependencies,
>> though I might be missing something.
>
> I'm just moving the 'HAS_IOPORT' dependency down from 
> 'COMEDI_PCI_DRIVERS' to its dependents.  Not all comedi PCI drivers use 
> I/O ports, although some of the drivers that do not use I/O ports do 
> depend on 'COMEDI_8254' and 'COMEDI_8255' which do depend on 'HAS_IOPORT'.
>
>> If this addresses another problem for you, maybe split it out
>> into a separate patch and describe why you move the dependencies.
>
> I'm just correcting one patch with one patch, so don't really want to 
> split it.  I could improve the patch description though.

No, you should always split bugfixes from cleanups, otherwise
it's impossible to review the patch. Please either revert the patch
and do it correctly the way you want, or send one bugfix patch
and a cleanup on top.

>> Are you trying to make sure that it's possible to build PCI
>> IIO drivers that don't depend on HAS_IOPORT on targets that
>> don't provide it?
>
> Yes (well, PCI comedi drivers rather than IIO drivers). 

sorry, my mistake.

>> It might be easier to revert the original patch, and then follow
>> up with a fixed version.
>
> Will any random config builds break in 6.5 stable if the original patch 
> is reverted, or is the 'HAS_IOPORT' stuff still in preparation for 
> future use?

No, the final patches to remove ioport stubs for !HAS_IOPORT configs
got delayed for other reasons and isn't part of 6.6-rc1 either, so
if you do a revert followed by annother patch in 6.6, backporting just
the revert should be fine.

     Arnd