[v8,01/25] scsi/atari_scsi: Don't select CONFIG_NVRAM

Commit Message

Finn Thain Dec. 26, 2018, 12:37 a.m. UTC

On powerpc, setting CONFIG_NVRAM=n builds a kernel with no NVRAM support.
Setting CONFIG_NVRAM=m enables the /dev/nvram misc device module without
enabling NVRAM support in drivers. Setting CONFIG_NVRAM=y enables the
misc device (built-in) and also enables NVRAM support in drivers.

m68k shares the valkyriefb driver with powerpc, and since that driver uses
NVRAM, it is affected by CONFIG_ATARI_SCSI, because of the use of
"select NVRAM".

Adopt the powerpc convention on m68k to avoid surprises.

Signed-off-by: Finn Thain <[e-mail]>
Tested-by: Christian T. Steigies <[e-mail]>
---
This patch temporarily disables CONFIG_NVRAM on Atari, to prevent build
failures when bisecting the rest of this patch series. It gets enabled
again with the introduction of CONFIG_HAVE_ARCH_NVRAM_OPS, once the
nvram_* global functions have been moved to an ops struct.
---
 drivers/char/Kconfig      | 5 +----
 drivers/scsi/Kconfig      | 6 +++---
 drivers/scsi/atari_scsi.c | 7 ++++---
 3 files changed, 8 insertions(+), 10 deletions(-)

Comments

Christophe Leroy Dec. 28, 2018, 4:38 p.m. UTC | #1

Finn Thain <[e-mail]> a écrit :

> On powerpc, setting CONFIG_NVRAM=n builds a kernel with no NVRAM support.
> Setting CONFIG_NVRAM=m enables the /dev/nvram misc device module without
> enabling NVRAM support in drivers. Setting CONFIG_NVRAM=y enables the
> misc device (built-in) and also enables NVRAM support in drivers.
>
> m68k shares the valkyriefb driver with powerpc, and since that driver uses
> NVRAM, it is affected by CONFIG_ATARI_SCSI, because of the use of
> "select NVRAM".
>
> Adopt the powerpc convention on m68k to avoid surprises.
>
> Signed-off-by: Finn Thain <[e-mail]>
> Tested-by: Christian T. Steigies <[e-mail]>
> ---
> This patch temporarily disables CONFIG_NVRAM on Atari, to prevent build
> failures when bisecting the rest of this patch series. It gets enabled
> again with the introduction of CONFIG_HAVE_ARCH_NVRAM_OPS, once the
> nvram_* global functions have been moved to an ops struct.
> ---
>  drivers/char/Kconfig      | 5 +----
>  drivers/scsi/Kconfig      | 6 +++---
>  drivers/scsi/atari_scsi.c | 7 ++++---
>  3 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index 9d03b2ff5df6..5b54595dfe30 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -236,7 +236,7 @@ source "drivers/char/hw_random/Kconfig"
>
>  config NVRAM
>  	tristate "/dev/nvram support"
> -	depends on ATARI || X86 || GENERIC_NVRAM
> +	depends on X86 || GENERIC_NVRAM
>  	---help---
>  	  If you say Y here and create a character special file /dev/nvram
>  	  with major number 10 and minor number 144 using mknod ("man mknod"),
> @@ -254,9 +254,6 @@ config NVRAM
>  	  should NEVER idly tamper with it. See Ralf Brown's interrupt list
>  	  for a guide to the use of CMOS bytes by your BIOS.
>
> -	  On Atari machines, /dev/nvram is always configured and does not need
> -	  to be selected.
> -
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called nvram.
>
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index 640cd1b31a18..924eb69e7fc4 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -1381,14 +1381,14 @@ config ATARI_SCSI
>  	tristate "Atari native SCSI support"
>  	depends on ATARI && SCSI
>  	select SCSI_SPI_ATTRS
> -	select NVRAM
>  	---help---
>  	  If you have an Atari with built-in NCR5380 SCSI controller (TT,
>  	  Falcon, ...) say Y to get it supported. Of course also, if you have
>  	  a compatible SCSI controller (e.g. for Medusa).
>
> -	  To compile this driver as a module, choose M here: the
> -	  module will be called atari_scsi.
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called atari_scsi. If you also enable NVRAM support, the SCSI
> +	  host's ID is taken from the setting in TT RTC NVRAM.
>
>  	  This driver supports both styles of NCR integration into the
>  	  system: the TT style (separate DMA), and the Falcon style (via
> diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
> index 89f5154c40b6..99e5729d910d 100644
> --- a/drivers/scsi/atari_scsi.c
> +++ b/drivers/scsi/atari_scsi.c
> @@ -755,9 +755,10 @@ static int __init atari_scsi_probe(struct  
> platform_device *pdev)
>  	if (ATARIHW_PRESENT(TT_SCSI) && setup_sg_tablesize >= 0)
>  		atari_scsi_template.sg_tablesize = setup_sg_tablesize;
>
> -	if (setup_hostid >= 0) {
> +	if (setup_hostid >= 0)
>  		atari_scsi_template.this_id = setup_hostid & 7;
> -	} else {
> +#ifdef CONFIG_NVRAM
> +	else

Such ifdefs should be avoided in C files.
It would be better to use

} else if (IS_ENABLED(CONFIG_NVRAM)) {

>  		/* Test if a host id is set in the NVRam */
>  		if (ATARIHW_PRESENT(TT_CLK) && nvram_check_checksum()) {
>  			unsigned char b = nvram_read_byte(16);
> @@ -768,7 +769,7 @@ static int __init atari_scsi_probe(struct  
> platform_device *pdev)
>  			if (b & 0x80)
>  				atari_scsi_template.this_id = b & 7;
>  		}
> -	}
> +#endif
>
>  	/* If running on a Falcon and if there's TT-Ram (i.e., more than one
>  	 * memory block, since there's always ST-Ram in a Falcon), then
> --
> 2.19.2

Finn Thain Dec. 29, 2018, 1:06 a.m. UTC | #2

On Fri, 28 Dec 2018, LEROY Christophe wrote:

> Finn Thain <[e-mail]> a ?crit?:
> 
> > On powerpc, setting CONFIG_NVRAM=n builds a kernel with no NVRAM support.
> > Setting CONFIG_NVRAM=m enables the /dev/nvram misc device module without
> > enabling NVRAM support in drivers. Setting CONFIG_NVRAM=y enables the
> > misc device (built-in) and also enables NVRAM support in drivers.
> >
> > m68k shares the valkyriefb driver with powerpc, and since that driver uses
> > NVRAM, it is affected by CONFIG_ATARI_SCSI, because of the use of
> > "select NVRAM".
> >
> > Adopt the powerpc convention on m68k to avoid surprises.
> >
> > Signed-off-by: Finn Thain <[e-mail]>
> > Tested-by: Christian T. Steigies <[e-mail]>
> > ---
> > This patch temporarily disables CONFIG_NVRAM on Atari, to prevent build
> > failures when bisecting the rest of this patch series. It gets enabled
> > again with the introduction of CONFIG_HAVE_ARCH_NVRAM_OPS, once the
> > nvram_* global functions have been moved to an ops struct.
> > ---
> >  drivers/char/Kconfig      | 5 +----
> >  drivers/scsi/Kconfig      | 6 +++---
> >  drivers/scsi/atari_scsi.c | 7 ++++---
> >  3 files changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> > index 9d03b2ff5df6..5b54595dfe30 100644
> > --- a/drivers/char/Kconfig
> > +++ b/drivers/char/Kconfig
> > @@ -236,7 +236,7 @@ source "drivers/char/hw_random/Kconfig"
> >
> >  config NVRAM
> >  	tristate "/dev/nvram support"
> > -	depends on ATARI || X86 || GENERIC_NVRAM
> > +	depends on X86 || GENERIC_NVRAM
> >  	---help---
> >  	  If you say Y here and create a character special file /dev/nvram
> >  	  with major number 10 and minor number 144 using mknod ("man mknod"),
> > @@ -254,9 +254,6 @@ config NVRAM
> >  	  should NEVER idly tamper with it. See Ralf Brown's interrupt list
> >  	  for a guide to the use of CMOS bytes by your BIOS.
> >
> > -	  On Atari machines, /dev/nvram is always configured and does not need
> > -	  to be selected.
> > -
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called nvram.
> >
> > diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> > index 640cd1b31a18..924eb69e7fc4 100644
> > --- a/drivers/scsi/Kconfig
> > +++ b/drivers/scsi/Kconfig
> > @@ -1381,14 +1381,14 @@ config ATARI_SCSI
> >  	tristate "Atari native SCSI support"
> >  	depends on ATARI && SCSI
> >  	select SCSI_SPI_ATTRS
> > -	select NVRAM
> >  	---help---
> >  	  If you have an Atari with built-in NCR5380 SCSI controller (TT,
> >  	  Falcon, ...) say Y to get it supported. Of course also, if you have
> >  	  a compatible SCSI controller (e.g. for Medusa).
> >
> > -	  To compile this driver as a module, choose M here: the
> > -	  module will be called atari_scsi.
> > +	  To compile this driver as a module, choose M here: the module will
> > +	  be called atari_scsi. If you also enable NVRAM support, the SCSI
> > +	  host's ID is taken from the setting in TT RTC NVRAM.
> >
> >  	  This driver supports both styles of NCR integration into the
> >  	  system: the TT style (separate DMA), and the Falcon style (via
> > diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
> > index 89f5154c40b6..99e5729d910d 100644
> > --- a/drivers/scsi/atari_scsi.c
> > +++ b/drivers/scsi/atari_scsi.c
> > @@ -755,9 +755,10 @@ static int __init atari_scsi_probe(struct  
> > platform_device *pdev)
> >  	if (ATARIHW_PRESENT(TT_SCSI) && setup_sg_tablesize >= 0)
> >  		atari_scsi_template.sg_tablesize = setup_sg_tablesize;
> >
> > -	if (setup_hostid >= 0) {
> > +	if (setup_hostid >= 0)
> >  		atari_scsi_template.this_id = setup_hostid & 7;
> > -	} else {
> > +#ifdef CONFIG_NVRAM
> > +	else
> 
> Such ifdefs should be avoided in C files.
> It would be better to use
> 
> } else if (IS_ENABLED(CONFIG_NVRAM)) {
> 

I don't like #ifdefs either. However, as the maintainer of this file, I am 
okay with this one.

The old #ifdef CONFIG_NVRAM conditional compilation convention that gets 
used here and under drivers/video/fbdev could probably be improved upon 
but I consider this to be out-of-scope for this series, which is 
complicated enough.

And as explained in the commit log, CONFIG_NVRAM=y and CONFIG_NVRAM=m are 
treaded differently by drivers. Therefore, IS_ENABLED would be incorrect.

Michael Schmitz Dec. 29, 2018, 1:33 a.m. UTC | #3

Hi Finn,

Am 29.12.2018 um 14:06 schrieb Finn Thain:
> On Fri, 28 Dec 2018, LEROY Christophe wrote:
>>> diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
>>> index 89f5154c40b6..99e5729d910d 100644
>>> --- a/drivers/scsi/atari_scsi.c
>>> +++ b/drivers/scsi/atari_scsi.c
>>> @@ -755,9 +755,10 @@ static int __init atari_scsi_probe(struct
>>> platform_device *pdev)
>>>  	if (ATARIHW_PRESENT(TT_SCSI) && setup_sg_tablesize >= 0)
>>>  		atari_scsi_template.sg_tablesize = setup_sg_tablesize;
>>>
>>> -	if (setup_hostid >= 0) {
>>> +	if (setup_hostid >= 0)
>>>  		atari_scsi_template.this_id = setup_hostid & 7;
>>> -	} else {
>>> +#ifdef CONFIG_NVRAM
>>> +	else
>>
>> Such ifdefs should be avoided in C files.
>> It would be better to use
>>
>> } else if (IS_ENABLED(CONFIG_NVRAM)) {
>>
>
> I don't like #ifdefs either. However, as the maintainer of this file, I am
> okay with this one.
>
> The old #ifdef CONFIG_NVRAM conditional compilation convention that gets
> used here and under drivers/video/fbdev could probably be improved upon
> but I consider this to be out-of-scope for this series, which is
> complicated enough.
>
> And as explained in the commit log, CONFIG_NVRAM=y and CONFIG_NVRAM=m are
> treaded differently by drivers. Therefore, IS_ENABLED would be incorrect.

IS_BUILTIN(CONFIG_NVRAM) is probably what Christophe really meant to 
suggest.

Or (really going out on a limb here):

IS_BUILTIN(CONFIG_NVRAM) ||
( IS_MODULE(CONFIG_ATARI_SCSI) && IS_ENABLED(CONFIG_NVRAM) )

Not that I'd advocate that, for this series.

Cheers,

	Michael

Finn Thain Dec. 29, 2018, 2:34 a.m. UTC | #4

On Sat, 29 Dec 2018, Michael Schmitz wrote:

> 
> IS_BUILTIN(CONFIG_NVRAM) is probably what Christophe really meant to suggest.
> 
> Or (really going out on a limb here):
> 
> IS_BUILTIN(CONFIG_NVRAM) ||
> ( IS_MODULE(CONFIG_ATARI_SCSI) && IS_ENABLED(CONFIG_NVRAM) )
> 
> Not that I'd advocate that, for this series.
> 

Well, you are a maintainer for atari_scsi.c.

Are you saying that you want IS_BUILTIN(CONFIG_NVRAM) used here instead of 
ifdef?

OTOH, if you approve of the existing patch, please send your acked-by.

Michael Schmitz Dec. 29, 2018, 2:50 a.m. UTC | #5

Hi Finn,

Am 29.12.2018 um 15:34 schrieb Finn Thain:
> On Sat, 29 Dec 2018, Michael Schmitz wrote:
>
>>
>> IS_BUILTIN(CONFIG_NVRAM) is probably what Christophe really meant to suggest.
>>
>> Or (really going out on a limb here):
>>
>> IS_BUILTIN(CONFIG_NVRAM) ||
>> ( IS_MODULE(CONFIG_ATARI_SCSI) && IS_ENABLED(CONFIG_NVRAM) )
>>
>> Not that I'd advocate that, for this series.
>>
>
> Well, you are a maintainer for atari_scsi.c.
>
> Are you saying that you want IS_BUILTIN(CONFIG_NVRAM) used here instead of
> ifdef?

No, just pointing out that there would be a way to avoid the ifdef 
without messing up driver behaviour. I'm fine with the ifdef - not least 
because it clearly eliminates code that would be unreachable.

(On second thought - I don't want to speculate whether there's weird 
compiler options that could result in the nvram_check_checksum and 
nvram_read_bytes symbols to still be referenced in the final link, even 
though IS_BUILTIN(CONFIG_NVRAM) always evaluates to false. Best leave 
this as-is.)

> OTOH, if you approve of the existing patch, please send your acked-by.

Of course - I'd seen Geert's acked-by on some of the patches and forgot 
to check which still required acks.

Cheers,

	Michael

Michael Schmitz Dec. 29, 2018, 2:54 a.m. UTC | #6

Hi Finn,

Am 26.12.2018 um 13:37 schrieb Finn Thain:
> On powerpc, setting CONFIG_NVRAM=n builds a kernel with no NVRAM support.
> Setting CONFIG_NVRAM=m enables the /dev/nvram misc device module without
> enabling NVRAM support in drivers. Setting CONFIG_NVRAM=y enables the
> misc device (built-in) and also enables NVRAM support in drivers.
>
> m68k shares the valkyriefb driver with powerpc, and since that driver uses
> NVRAM, it is affected by CONFIG_ATARI_SCSI, because of the use of
> "select NVRAM".
>
> Adopt the powerpc convention on m68k to avoid surprises.
>
> Signed-off-by: Finn Thain <[e-mail]>
> Tested-by: Christian T. Steigies <[e-mail]>

Acked-by: Michael Schmitz <[e-mail]>

> ---
> This patch temporarily disables CONFIG_NVRAM on Atari, to prevent build
> failures when bisecting the rest of this patch series. It gets enabled
> again with the introduction of CONFIG_HAVE_ARCH_NVRAM_OPS, once the
> nvram_* global functions have been moved to an ops struct.
> ---
>  drivers/char/Kconfig      | 5 +----
>  drivers/scsi/Kconfig      | 6 +++---
>  drivers/scsi/atari_scsi.c | 7 ++++---
>  3 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index 9d03b2ff5df6..5b54595dfe30 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -236,7 +236,7 @@ source "drivers/char/hw_random/Kconfig"
>
>  config NVRAM
>  	tristate "/dev/nvram support"
> -	depends on ATARI || X86 || GENERIC_NVRAM
> +	depends on X86 || GENERIC_NVRAM
>  	---help---
>  	  If you say Y here and create a character special file /dev/nvram
>  	  with major number 10 and minor number 144 using mknod ("man mknod"),
> @@ -254,9 +254,6 @@ config NVRAM
>  	  should NEVER idly tamper with it. See Ralf Brown's interrupt list
>  	  for a guide to the use of CMOS bytes by your BIOS.
>
> -	  On Atari machines, /dev/nvram is always configured and does not need
> -	  to be selected.
> -
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called nvram.
>
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index 640cd1b31a18..924eb69e7fc4 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -1381,14 +1381,14 @@ config ATARI_SCSI
>  	tristate "Atari native SCSI support"
>  	depends on ATARI && SCSI
>  	select SCSI_SPI_ATTRS
> -	select NVRAM
>  	---help---
>  	  If you have an Atari with built-in NCR5380 SCSI controller (TT,
>  	  Falcon, ...) say Y to get it supported. Of course also, if you have
>  	  a compatible SCSI controller (e.g. for Medusa).
>
> -	  To compile this driver as a module, choose M here: the
> -	  module will be called atari_scsi.
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called atari_scsi. If you also enable NVRAM support, the SCSI
> +	  host's ID is taken from the setting in TT RTC NVRAM.
>
>  	  This driver supports both styles of NCR integration into the
>  	  system: the TT style (separate DMA), and the Falcon style (via
> diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
> index 89f5154c40b6..99e5729d910d 100644
> --- a/drivers/scsi/atari_scsi.c
> +++ b/drivers/scsi/atari_scsi.c
> @@ -755,9 +755,10 @@ static int __init atari_scsi_probe(struct platform_device *pdev)
>  	if (ATARIHW_PRESENT(TT_SCSI) && setup_sg_tablesize >= 0)
>  		atari_scsi_template.sg_tablesize = setup_sg_tablesize;
>
> -	if (setup_hostid >= 0) {
> +	if (setup_hostid >= 0)
>  		atari_scsi_template.this_id = setup_hostid & 7;
> -	} else {
> +#ifdef CONFIG_NVRAM
> +	else
>  		/* Test if a host id is set in the NVRam */
>  		if (ATARIHW_PRESENT(TT_CLK) && nvram_check_checksum()) {
>  			unsigned char b = nvram_read_byte(16);
> @@ -768,7 +769,7 @@ static int __init atari_scsi_probe(struct platform_device *pdev)
>  			if (b & 0x80)
>  				atari_scsi_template.this_id = b & 7;
>  		}
> -	}
> +#endif
>
>  	/* If running on a Falcon and if there's TT-Ram (i.e., more than one
>  	 * memory block, since there's always ST-Ram in a Falcon), then
>

Christophe Leroy Dec. 29, 2018, 4:55 p.m. UTC | #7

Michael Schmitz <[e-mail]> a écrit :

> Hi Finn,
>
> Am 29.12.2018 um 14:06 schrieb Finn Thain:
>> On Fri, 28 Dec 2018, LEROY Christophe wrote:
>>>> diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
>>>> index 89f5154c40b6..99e5729d910d 100644
>>>> --- a/drivers/scsi/atari_scsi.c
>>>> +++ b/drivers/scsi/atari_scsi.c
>>>> @@ -755,9 +755,10 @@ static int __init atari_scsi_probe(struct
>>>> platform_device *pdev)
>>>> 	if (ATARIHW_PRESENT(TT_SCSI) && setup_sg_tablesize >= 0)
>>>> 		atari_scsi_template.sg_tablesize = setup_sg_tablesize;
>>>>
>>>> -	if (setup_hostid >= 0) {
>>>> +	if (setup_hostid >= 0)
>>>> 		atari_scsi_template.this_id = setup_hostid & 7;
>>>> -	} else {
>>>> +#ifdef CONFIG_NVRAM
>>>> +	else
>>>
>>> Such ifdefs should be avoided in C files.
>>> It would be better to use
>>>
>>> } else if (IS_ENABLED(CONFIG_NVRAM)) {
>>>
>>
>> I don't like #ifdefs either. However, as the maintainer of this file, I am
>> okay with this one.
>>
>> The old #ifdef CONFIG_NVRAM conditional compilation convention that gets
>> used here and under drivers/video/fbdev could probably be improved upon
>> but I consider this to be out-of-scope for this series, which is
>> complicated enough.
>>
>> And as explained in the commit log, CONFIG_NVRAM=y and CONFIG_NVRAM=m are
>> treaded differently by drivers. Therefore, IS_ENABLED would be incorrect.
>
> IS_BUILTIN(CONFIG_NVRAM) is probably what Christophe really meant to suggest.

Doesn't #ifdef means either y or m ? So the same as IS_ENABLED() ?

Christophe

>
> Or (really going out on a limb here):
>
> IS_BUILTIN(CONFIG_NVRAM) ||
> ( IS_MODULE(CONFIG_ATARI_SCSI) && IS_ENABLED(CONFIG_NVRAM) )
>
> Not that I'd advocate that, for this series.
>
> Cheers,
>
> 	Michael

Michael Schmitz Dec. 29, 2018, 6:54 p.m. UTC | #8

Christophe,

Am 30.12.2018 um 05:55 schrieb LEROY Christophe:
> Michael Schmitz <[e-mail]> a écrit :
>
>> Hi Finn,
>>
>> Am 29.12.2018 um 14:06 schrieb Finn Thain:
>>> On Fri, 28 Dec 2018, LEROY Christophe wrote:
>>>>> diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
>>>>> index 89f5154c40b6..99e5729d910d 100644
>>>>> --- a/drivers/scsi/atari_scsi.c
>>>>> +++ b/drivers/scsi/atari_scsi.c
>>>>> @@ -755,9 +755,10 @@ static int __init atari_scsi_probe(struct
>>>>> platform_device *pdev)
>>>>>     if (ATARIHW_PRESENT(TT_SCSI) && setup_sg_tablesize >= 0)
>>>>>         atari_scsi_template.sg_tablesize = setup_sg_tablesize;
>>>>>
>>>>> -    if (setup_hostid >= 0) {
>>>>> +    if (setup_hostid >= 0)
>>>>>         atari_scsi_template.this_id = setup_hostid & 7;
>>>>> -    } else {
>>>>> +#ifdef CONFIG_NVRAM
>>>>> +    else
>>>>
>>>> Such ifdefs should be avoided in C files.
>>>> It would be better to use
>>>>
>>>> } else if (IS_ENABLED(CONFIG_NVRAM)) {
>>>>
>>>
>>> I don't like #ifdefs either. However, as the maintainer of this file,
>>> I am
>>> okay with this one.
>>>
>>> The old #ifdef CONFIG_NVRAM conditional compilation convention that gets
>>> used here and under drivers/video/fbdev could probably be improved upon
>>> but I consider this to be out-of-scope for this series, which is
>>> complicated enough.
>>>
>>> And as explained in the commit log, CONFIG_NVRAM=y and CONFIG_NVRAM=m
>>> are
>>> treaded differently by drivers. Therefore, IS_ENABLED would be
>>> incorrect.
>>
>> IS_BUILTIN(CONFIG_NVRAM) is probably what Christophe really meant to
>> suggest.
>
> Doesn't #ifdef means either y or m ? So the same as IS_ENABLED() ?

#ifdef CONFIG_NVRAM is used if you want to match CONFIG_NVRAM=y. For 
CONFIG_NVRAM=m, you'd use #ifdef CONFIG_NVRAM_MODULE.

Cheers,

	Michael


>
> Christophe
>
>>
>> Or (really going out on a limb here):
>>
>> IS_BUILTIN(CONFIG_NVRAM) ||
>> ( IS_MODULE(CONFIG_ATARI_SCSI) && IS_ENABLED(CONFIG_NVRAM) )
>>
>> Not that I'd advocate that, for this series.
>>
>> Cheers,
>>
>>     Michael
>
>

Arnd Bergmann Dec. 29, 2018, 9:37 p.m. UTC | #9

On Sat, Dec 29, 2018 at 3:51 AM Michael Schmitz <[e-mail]> wrote:
>
> Hi Finn,
>
> Am 29.12.2018 um 15:34 schrieb Finn Thain:
> > On Sat, 29 Dec 2018, Michael Schmitz wrote:
> >
> >>
> >> IS_BUILTIN(CONFIG_NVRAM) is probably what Christophe really meant to suggest.
> >>
> >> Or (really going out on a limb here):
> >>
> >> IS_BUILTIN(CONFIG_NVRAM) ||
> >> ( IS_MODULE(CONFIG_ATARI_SCSI) && IS_ENABLED(CONFIG_NVRAM) )
> >>
> >> Not that I'd advocate that, for this series.
> >>
> >
> > Well, you are a maintainer for atari_scsi.c.
> >
> > Are you saying that you want IS_BUILTIN(CONFIG_NVRAM) used here instead of
> > ifdef?
>
> No, just pointing out that there would be a way to avoid the ifdef
> without messing up driver behaviour. I'm fine with the ifdef - not least
> because it clearly eliminates code that would be unreachable.
>
> (On second thought - I don't want to speculate whether there's weird
> compiler options that could result in the nvram_check_checksum and
> nvram_read_bytes symbols to still be referenced in the final link, even
> though IS_BUILTIN(CONFIG_NVRAM) always evaluates to false. Best leave
> this as-is.)

As far as I know, it's totally reliable with the supported compilers (gcc-4.6+).
In the older compilers (e.g. 4.1), there was a corner case, where it could
have failed to eliminate a function that was only referenced through a pointer
from a discarded variable, but a plain IS_ENABLED() check like the one here
was still ok, and lots of code relies on that.

Other than that, I agree either way is totally fine here, so no objections
to using the #ifdef.

      Arnd

Christophe Leroy Dec. 30, 2018, 5:50 p.m. UTC | #10

Arnd Bergmann <[e-mail]> a écrit :

> On Sat, Dec 29, 2018 at 3:51 AM Michael Schmitz <[e-mail]> wrote:
>>
>> Hi Finn,
>>
>> Am 29.12.2018 um 15:34 schrieb Finn Thain:
>> > On Sat, 29 Dec 2018, Michael Schmitz wrote:
>> >
>> >>
>> >> IS_BUILTIN(CONFIG_NVRAM) is probably what Christophe really  
>> meant to suggest.
>> >>
>> >> Or (really going out on a limb here):
>> >>
>> >> IS_BUILTIN(CONFIG_NVRAM) ||
>> >> ( IS_MODULE(CONFIG_ATARI_SCSI) && IS_ENABLED(CONFIG_NVRAM) )
>> >>
>> >> Not that I'd advocate that, for this series.
>> >>
>> >
>> > Well, you are a maintainer for atari_scsi.c.
>> >
>> > Are you saying that you want IS_BUILTIN(CONFIG_NVRAM) used here instead of
>> > ifdef?
>>
>> No, just pointing out that there would be a way to avoid the ifdef
>> without messing up driver behaviour. I'm fine with the ifdef - not least
>> because it clearly eliminates code that would be unreachable.
>>
>> (On second thought - I don't want to speculate whether there's weird
>> compiler options that could result in the nvram_check_checksum and
>> nvram_read_bytes symbols to still be referenced in the final link, even
>> though IS_BUILTIN(CONFIG_NVRAM) always evaluates to false. Best leave
>> this as-is.)
>
> As far as I know, it's totally reliable with the supported compilers  
> (gcc-4.6+).
> In the older compilers (e.g. 4.1), there was a corner case, where it could
> have failed to eliminate a function that was only referenced through  
> a pointer
> from a discarded variable, but a plain IS_ENABLED() check like the one here
> was still ok, and lots of code relies on that.
>
> Other than that, I agree either way is totally fine here, so no objections
> to using the #ifdef.

As far as I know, kernel codying style promotes the use of  
IS_ENABLED() etc. instead of #ifdefs when possible.

Christophe

>
>       Arnd

James Bottomley Dec. 30, 2018, 6:06 p.m. UTC | #11

On Sun, 2018-12-30 at 18:50 +0100, LEROY Christophe wrote:
> Arnd Bergmann <[e-mail]> a écrit :
> > On Sat, Dec 29, 2018 at 3:51 AM Michael Schmitz
> > <[e-mail]> wrote:
[...]
> > > (On second thought - I don't want to speculate whether there's
> > > weird compiler options that could result in the
> > > nvram_check_checksum and nvram_read_bytes symbols to still be
> > > referenced in the final link, even though
> > > IS_BUILTIN(CONFIG_NVRAM) always evaluates to false. Best
> > > leave this as-is.)
> > 
> > As far as I know, it's totally reliable with the supported
> > compilers  (gcc-4.6+). In the older compilers (e.g. 4.1), there was
> > a corner case, where it could have failed to eliminate a function
> > that was only referenced through a pointer from a discarded
> > variable, but a plain IS_ENABLED() check like the one here
> > was still ok, and lots of code relies on that.
> > 
> > Other than that, I agree either way is totally fine here, so no
> > objections to using the #ifdef.
> 
> As far as I know, kernel codying style promotes the use of  
> IS_ENABLED() etc. instead of #ifdefs when possible.

It's a preference, as with a lot of coding style stuff, which we leave
up to the maintainer.

That said, as has been pointed out, the current #ifdef has a failing
corner case when both are modular (because the code should then be
included).  The runtime macro that correctly expresses this is
IS_REACHABLE(CONFIG_NVRAM).

James

Finn Thain Dec. 30, 2018, 9:44 p.m. UTC | #12

On Sun, 30 Dec 2018, James Bottomley wrote:

> 
> That said, as has been pointed out, the current #ifdef has a failing 
> corner case when both are modular (because the code should then be 
> included).  The runtime macro that correctly expresses this is 
> IS_REACHABLE(CONFIG_NVRAM).
> 

No, in the case of CONFIG_NVRAM=m, the conditional code is deliberately 
excluded. This is discussed in the patch description. The convention on 
PPC32 is that device drivers drop support for NVRAM in this situation. 

I've adopted the PPC32 convention here because M68K drivers and PPC32 
drivers have to co-exist (I'm thinking of valkyriefb, but there are other 
examples).

Finn Thain Dec. 30, 2018, 10:45 p.m. UTC | #13

On Mon, 31 Dec 2018, Finn Thain wrote:

> On Sun, 30 Dec 2018, James Bottomley wrote:
> 
> > 
> > That said, as has been pointed out, the current #ifdef has a failing 
> > corner case when both are modular (because the code should then be 
> > included).  The runtime macro that correctly expresses this is 
> > IS_REACHABLE(CONFIG_NVRAM).
> > 
> 
> No, in the case of CONFIG_NVRAM=m, the conditional code is deliberately 
> excluded. This is discussed in the patch description. The convention on 
> PPC32 is that device drivers drop support for NVRAM in this situation. 
> 
> I've adopted the PPC32 convention here because M68K drivers and PPC32 
> drivers have to co-exist (I'm thinking of valkyriefb, but there are other 
> examples).
> 

I agree with your comment in principle, that these drivers should be using 
IS_REACHABLE(CONFIG_NVRAM), not defined(CONFIG_NVRAM).

$ grep -lrw CONFIG_NVRAM drivers/
drivers/video/fbdev/matrox/matroxfb_base.c
drivers/video/fbdev/platinumfb.c
drivers/video/fbdev/controlfb.c
drivers/video/fbdev/valkyriefb.c
drivers/video/fbdev/imsttfb.c
drivers/scsi/atari_scsi.c
$ 

But I think this is not the fault of this patch; it's just a historical 
accident.

Having said that, I will rework this series to convert all of the above 
drivers to IS_REACHABLE(CONFIG_NVRAM). I think it would be an improvement.

Besides, it seems likely that some rework involving ppc_md will be needed 
too because as Arnd pointed out, it would be good to avoid two sets of 
nvram accessor methods.

--

Patch

diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index 9d03b2ff5df6..5b54595dfe30 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -236,7 +236,7 @@  source "drivers/char/hw_random/Kconfig"
 
 config NVRAM
 	tristate "/dev/nvram support"
-	depends on ATARI || X86 || GENERIC_NVRAM
+	depends on X86 || GENERIC_NVRAM
 	---help---
 	  If you say Y here and create a character special file /dev/nvram
 	  with major number 10 and minor number 144 using mknod ("man mknod"),
@@ -254,9 +254,6 @@  config NVRAM
 	  should NEVER idly tamper with it. See Ralf Brown's interrupt list
 	  for a guide to the use of CMOS bytes by your BIOS.
 
-	  On Atari machines, /dev/nvram is always configured and does not need
-	  to be selected.
-
 	  To compile this driver as a module, choose M here: the
 	  module will be called nvram.
 
diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 640cd1b31a18..924eb69e7fc4 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -1381,14 +1381,14 @@  config ATARI_SCSI
 	tristate "Atari native SCSI support"
 	depends on ATARI && SCSI
 	select SCSI_SPI_ATTRS
-	select NVRAM
 	---help---
 	  If you have an Atari with built-in NCR5380 SCSI controller (TT,
 	  Falcon, ...) say Y to get it supported. Of course also, if you have
 	  a compatible SCSI controller (e.g. for Medusa).
 
-	  To compile this driver as a module, choose M here: the
-	  module will be called atari_scsi.
+	  To compile this driver as a module, choose M here: the module will
+	  be called atari_scsi. If you also enable NVRAM support, the SCSI
+	  host's ID is taken from the setting in TT RTC NVRAM.
 
 	  This driver supports both styles of NCR integration into the
 	  system: the TT style (separate DMA), and the Falcon style (via
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index 89f5154c40b6..99e5729d910d 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -755,9 +755,10 @@  static int __init atari_scsi_probe(struct platform_device *pdev)
 	if (ATARIHW_PRESENT(TT_SCSI) && setup_sg_tablesize >= 0)
 		atari_scsi_template.sg_tablesize = setup_sg_tablesize;
 
-	if (setup_hostid >= 0) {
+	if (setup_hostid >= 0)
 		atari_scsi_template.this_id = setup_hostid & 7;
-	} else {
+#ifdef CONFIG_NVRAM
+	else
 		/* Test if a host id is set in the NVRam */
 		if (ATARIHW_PRESENT(TT_CLK) && nvram_check_checksum()) {
 			unsigned char b = nvram_read_byte(16);
@@ -768,7 +769,7 @@  static int __init atari_scsi_probe(struct platform_device *pdev)
 			if (b & 0x80)
 				atari_scsi_template.this_id = b & 7;
 		}
-	}
+#endif
 
 	/* If running on a Falcon and if there's TT-Ram (i.e., more than one
 	 * memory block, since there's always ST-Ram in a Falcon), then