Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i36794607:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-701-g9b2f44d3ee-fm-20230823.001-g9b2f44d3
Mime-Version: 1.0
Message-Id: <33c2292b-08cb-44c7-9438-07d4060976ab@app.fastmail.com>
In-Reply-To: <20230901192615.89591-1-abbotti@mev.co.uk>
References: <20230901192615.89591-1-abbotti@mev.co.uk>
Date:   Sun, 03 Sep 2023 11:49:08 -0400
From:   "Arnd Bergmann" <arnd@kernel.org>
To:     "Ian Abbott" <abbotti@mev.co.uk>, linux-kernel@vger.kernel.org
Cc:     "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
        "Hartley Sweeten" <hsweeten@visionengravers.com>,
        "Niklas Schnelle" <schnelle@linux.ibm.com>, stable@vger.kernel.org
Subject: Re: [PATCH] comedi: Fix driver module dependencies since HAS_IOPORT changes
Content-Type: text/plain
Precedence: bulk

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.

> @@ -512,7 +500,7 @@ config COMEDI_NI_ATMIO16D
> 
>  config COMEDI_NI_LABPC_ISA
>  	tristate "NI Lab-PC and compatibles ISA support"
> -	depends on COMEDI_NI_LABPC
> +	select COMEDI_NI_LABPC
>  	help
>  	  Enable support for National Instruments Lab-PC and compatibles
>  	  Lab-PC-1200, Lab-PC-1200AI, Lab-PC+.

I was confused a bit by this, as the changelog doesn't mention
COMEDI_NI_LABPC, but I saw that this needs the same change
recursively, same as COMEDI_DAS08.

> @@ -576,7 +564,7 @@ endif # COMEDI_ISA_DRIVERS
> 
>  menuconfig COMEDI_PCI_DRIVERS
>  	tristate "Comedi PCI drivers"
> -	depends on PCI && HAS_IOPORT
> +	depends on PCI
>  	help
>  	  Enable support for comedi PCI drivers.
>
> @@ -587,6 +575,7 @@ if COMEDI_PCI_DRIVERS
> 
>  config COMEDI_8255_PCI
>  	tristate "Generic PCI based 8255 digital i/o board support"
> +	depends on HAS_IOPORT
>  	select COMEDI_8255
>  	help
>  	  Enable support for PCI based 8255 digital i/o boards. This driver

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.

If this addresses another problem for you, maybe split it out
into a separate patch and describe why you move the dependencies.

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?

> @@ -735,8 +738,8 @@ config COMEDI_ADL_PCI9111
> 
>  config COMEDI_ADL_PCI9118
>  	tristate "ADLink PCI-9118DG, PCI-9118HG, PCI-9118HR support"
> +	depends on HAS_IOPORT
>  	depends on HAS_DMA
> -	depends on COMEDI_8254
>  	help
>  	  Enable support for ADlink PCI-9118DG, PCI-9118HG, PCI-9118HR cards

I don't see why you'd remove the 'depends on COMEDI_8254' here
rather than turning it back into 'select' as it was originally.

It might be easier to revert the original patch, and then follow
up with a fixed version.

      Arnd