[PATCH v8 03/25] m68k/atari: Replace nvram_{read,write}_byte with arch_nvram_ops

[PATCH v8 03/25] m68k/atari: Replace nvram_{read,write}_byte with arch_nvram_ops

[Finn Thain]

By implementing an arch_nvram_ops struct, any platform can re-use the
drivers/char/nvram.c module without needing any arch-specific code
in that module. Atari does so here.

Atari has one user of nvram_check_checksum() whereas the other "CMOS"
platforms don't use that function at all. Replace this
validate-checksum-and-read-byte sequence with the equivalent
rtc_nvram_ops.read() call and remove the now unused functions.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Tested-by: Christian T. Steigies <cts@debian.org>
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
The advantage of the new ops struct over the old global nvram_* functions
is that the misc device module can be shared by different platforms
without requiring every platform to implement every nvram_* function.
E.g. only RTC "CMOS" NVRAMs have a checksum for the entire NVRAM
and only PowerPC platforms have a "sync" ioctl.
---
 arch/m68k/atari/nvram.c   | 89 ++++++++++++++++++++++++---------------
 drivers/scsi/atari_scsi.c |  8 ++--
 include/linux/nvram.h     |  9 ++++
 3 files changed, 70 insertions(+), 36 deletions(-)

diff --git a/arch/m68k/atari/nvram.c b/arch/m68k/atari/nvram.c
index 3e620ee955ba..bafc9dc32830 100644
--- a/arch/m68k/atari/nvram.c
+++ b/arch/m68k/atari/nvram.c
@@ -39,33 +39,12 @@ unsigned char __nvram_read_byte(int i)
 	return CMOS_READ(NVRAM_FIRST_BYTE + i);
 }
 
-unsigned char nvram_read_byte(int i)
-{
-	unsigned long flags;
-	unsigned char c;
-
-	spin_lock_irqsave(&rtc_lock, flags);
-	c = __nvram_read_byte(i);
-	spin_unlock_irqrestore(&rtc_lock, flags);
-	return c;
-}
-EXPORT_SYMBOL(nvram_read_byte);
-
 /* This races nicely with trying to read with checksum checking */
 void __nvram_write_byte(unsigned char c, int i)
 {
 	CMOS_WRITE(c, NVRAM_FIRST_BYTE + i);
 }
 
-void nvram_write_byte(unsigned char c, int i)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&rtc_lock, flags);
-	__nvram_write_byte(c, i);
-	spin_unlock_irqrestore(&rtc_lock, flags);
-}
-
 /* On Ataris, the checksum is over all bytes except the checksum bytes
  * themselves; these are at the very end.
  */
@@ -84,18 +63,6 @@ int __nvram_check_checksum(void)
 	       (__nvram_read_byte(ATARI_CKS_LOC + 1) == (sum & 0xff));
 }
 
-int nvram_check_checksum(void)
-{
-	unsigned long flags;
-	int rv;
-
-	spin_lock_irqsave(&rtc_lock, flags);
-	rv = __nvram_check_checksum();
-	spin_unlock_irqrestore(&rtc_lock, flags);
-	return rv;
-}
-EXPORT_SYMBOL(nvram_check_checksum);
-
 static void __nvram_set_checksum(void)
 {
 	int i;
@@ -107,6 +74,62 @@ static void __nvram_set_checksum(void)
 	__nvram_write_byte(sum, ATARI_CKS_LOC + 1);
 }
 
+static ssize_t nvram_read(char *buf, size_t count, loff_t *ppos)
+{
+	char *p = buf;
+	loff_t i;
+
+	spin_lock_irq(&rtc_lock);
+
+	if (!__nvram_check_checksum()) {
+		spin_unlock_irq(&rtc_lock);
+		return -EIO;
+	}
+
+	for (i = *ppos; count > 0 && i < NVRAM_BYTES; --count, ++i, ++p)
+		*p = __nvram_read_byte(i);
+
+	spin_unlock_irq(&rtc_lock);
+
+	*ppos = i;
+	return p - buf;
+}
+
+static ssize_t nvram_write(char *buf, size_t count, loff_t *ppos)
+{
+	char *p = buf;
+	loff_t i;
+
+	spin_lock_irq(&rtc_lock);
+
+	if (!__nvram_check_checksum()) {
+		spin_unlock_irq(&rtc_lock);
+		return -EIO;
+	}
+
+	for (i = *ppos; count > 0 && i < NVRAM_BYTES; --count, ++i, ++p)
+		__nvram_write_byte(*p, i);
+
+	__nvram_set_checksum();
+
+	spin_unlock_irq(&rtc_lock);
+
+	*ppos = i;
+	return p - buf;
+}
+
+static ssize_t nvram_get_size(void)
+{
+	return NVRAM_BYTES;
+}
+
+const struct nvram_ops arch_nvram_ops = {
+	.read           = nvram_read,
+	.write          = nvram_write,
+	.get_size       = nvram_get_size,
+};
+EXPORT_SYMBOL(arch_nvram_ops);
+
 #ifdef CONFIG_PROC_FS
 static struct {
 	unsigned char val;
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index 99e5729d910d..f55d13e400da 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -760,13 +760,15 @@ static int __init atari_scsi_probe(struct platform_device *pdev)
 #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);
+		if (ATARIHW_PRESENT(TT_CLK)) {
+			unsigned char b;
+			loff_t offset = 16;
+			ssize_t count = arch_nvram_ops.read(&b, 1, &offset);
 
 			/* Arbitration enabled? (for TOS)
 			 * If yes, use configured host ID
 			 */
-			if (b & 0x80)
+			if ((count == 1) && (b & 0x80))
 				atari_scsi_template.this_id = b & 7;
 		}
 #endif
diff --git a/include/linux/nvram.h b/include/linux/nvram.h
index 28bfb9ab94ca..4f78147e74d9 100644
--- a/include/linux/nvram.h
+++ b/include/linux/nvram.h
@@ -11,4 +11,13 @@ extern void __nvram_write_byte(unsigned char c, int i);
 extern void nvram_write_byte(unsigned char c, int i);
 extern int __nvram_check_checksum(void);
 extern int nvram_check_checksum(void);
+
+struct nvram_ops {
+	ssize_t         (*read)(char *, size_t, loff_t *);
+	ssize_t         (*write)(char *, size_t, loff_t *);
+	ssize_t         (*get_size)(void);
+};
+
+extern const struct nvram_ops arch_nvram_ops;
+
 #endif  /* _LINUX_NVRAM_H */
-- 
2.19.2

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

[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 <fthain@telegraphics.com.au>
Tested-by: Christian T. Steigies <cts@debian.org>
---
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
-- 
2.19.2

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

[LEROY Christophe]

Finn Thain <fthain@telegraphics.com.au> 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 <fthain@telegraphics.com.au>
> Tested-by: Christian T. Steigies <cts@debian.org>
> ---
> 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

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

[Finn Thain]

On Fri, 28 Dec 2018, LEROY Christophe wrote:

> Finn Thain <fthain@telegraphics.com.au> 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 <fthain@telegraphics.com.au>
> > Tested-by: Christian T. Steigies <cts@debian.org>
> > ---
> > 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.

-- 

> >  		/* 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
> 
> 
> 

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

[Michael Schmitz]

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

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

[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?

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

-- 

> Cheers,
> 
> 	Michael
> 
> 
> 
> 

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

[Michael Schmitz]

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

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

[Michael Schmitz]

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 <fthain@telegraphics.com.au>
> Tested-by: Christian T. Steigies <cts@debian.org>

Acked-by: Michael Schmitz <schmitzmic@gmail.com>

> ---
> 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
>

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

[LEROY Christophe]

Michael Schmitz <schmitzmic@gmail.com> 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

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

[Michael Schmitz]

Christophe,

Am 30.12.2018 um 05:55 schrieb LEROY Christophe:
> Michael Schmitz <schmitzmic@gmail.com> 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
>
>

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

[Arnd Bergmann]

On Sat, Dec 29, 2018 at 3:51 AM Michael Schmitz <schmitzmic@gmail.com> 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

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

[LEROY Christophe]

Arnd Bergmann <arnd@arndb.de> a écrit :

> On Sat, Dec 29, 2018 at 3:51 AM Michael Schmitz <schmitzmic@gmail.com> 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

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

[James Bottomley]

On Sun, 2018-12-30 at 18:50 +0100, LEROY Christophe wrote:
> Arnd Bergmann <arnd@arndb.de> a écrit :
> > On Sat, Dec 29, 2018 at 3:51 AM Michael Schmitz
> > <schmitzmic@gmail.com> 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

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

[Finn Thain]

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).

-- 

> James
> 
> 

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

[Finn Thain]

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.

--