subject:"\[PATCH 3\/3\] g_NCR5380\: Stop using scsi

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

2016-09-28 Thread Finn Thain


Acked-by: Finn Thain 

On Tue, 27 Sep 2016, Ondrej Zary wrote:

> Convert g_NCR5380 to use scsi_add_host instead of scsi_module.c
> Use pnp_driver and isa_driver to manage cards.
> 
> In order to support multiple cards, new module parameter format is
> introduced. The old parameters are kept for compatibility.
> 
> Signed-off-by: Ondrej Zary 
> ---
>  Documentation/scsi/g_NCR5380.txt |   24 ++-
>  drivers/scsi/g_NCR5380.c |  335 
> ++
>  drivers/scsi/g_NCR5380.h |8 -
>  3 files changed, 215 insertions(+), 152 deletions(-)
> 
> diff --git a/Documentation/scsi/g_NCR5380.txt 
> b/Documentation/scsi/g_NCR5380.txt
> index 843cbae..e2c1879 100644
> --- a/Documentation/scsi/g_NCR5380.txt
> +++ b/Documentation/scsi/g_NCR5380.txt
> @@ -28,6 +28,16 @@ time. More info to come in the future.
>  This driver works as a module.
>  When included as a module, parameters can be passed on the insmod/modprobe
>  command line:
> +  irq=xx[,...]   the interrupt(s)
> +  base=xx[,...]  the port or base address(es) (for port or memory 
> mapped, resp.)
> +  card=xx[,...]  card type(s):
> + 0 = NCR5380,
> + 1 = NCR53C400,
> + 2 = NCR53C400A,
> + 3 = Domex Technology Corp 3181E (DTC3181E)
> + 4 = Hewlett Packard C2502
> +
> +These old-style parameters can support only one card:
>ncr_irq=xx   the interrupt
>ncr_addr=xx  the port or base address (for port or memory
> mapped, resp.)
> @@ -36,11 +46,19 @@ command line:
>ncr_53c400a=1 to set up for a NCR53C400A board
>dtc_3181e=1  to set up for a Domex Technology Corp 3181E board
>hp_c2502=1   to set up for a Hewlett Packard C2502 board
> +
>  e.g.
> -modprobe g_NCR5380 ncr_irq=5 ncr_addr=0x350 ncr_5380=1
> +OLD: modprobe g_NCR5380 ncr_irq=5 ncr_addr=0x350 ncr_5380=1
> +NEW: modprobe g_NCR5380 irq=5 base=0x350 card=0
>for a port mapped NCR5380 board or
> -modprobe g_NCR5380 ncr_irq=255 ncr_addr=0xc8000 ncr_53c400=1
> -  for a memory mapped NCR53C400 board with interrupts disabled.
> +
> +OLD: modprobe g_NCR5380 ncr_irq=255 ncr_addr=0xc8000 ncr_53c400=1
> +NEW: modprobe g_NCR5380 irq=255 base=0xc8000 card=1
> +  for a memory mapped NCR53C400 board with interrupts disabled or
> +
> +NEW: modprobe g_NCR5380 irq=0,7 base=0x240,0x300 card=3,4
> +  for two cards: DTC3181 (in non-PnP mode) at 0x240 with no IRQ
> + and HP C2502 at 0x300 with IRQ 7
>  
>  (255 should be specified for no or DMA interrupt, 254 to autoprobe for an 
>   IRQ line if overridden on the command line.)
> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 5162de6..cbf0103 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -30,24 +30,41 @@
>  #include "NCR5380.h"
>  #include 
>  #include 
> -#include 
> +#include 
> +#include 
>  #include 
>  
> +#define MAX_CARDS 8
> +
> +/* old-style parameters for compatibility */
>  static int ncr_irq;
> -static int ncr_dma;
>  static int ncr_addr;
>  static int ncr_5380;
>  static int ncr_53c400;
>  static int ncr_53c400a;
>  static int dtc_3181e;
>  static int hp_c2502;
> +module_param(ncr_irq, int, 0);
> +module_param(ncr_addr, int, 0);
> +module_param(ncr_5380, int, 0);
> +module_param(ncr_53c400, int, 0);
> +module_param(ncr_53c400a, int, 0);
> +module_param(dtc_3181e, int, 0);
> +module_param(hp_c2502, int, 0);
> +
> +static int irq[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
> +module_param_array(irq, int, NULL, 0);
> +MODULE_PARM_DESC(irq, "IRQ number(s)");
>  
> -static struct card {
> - NCR5380_map_type NCR5380_map_name;
> - int irq;
> - int dma;
> - int board;  /* Use NCR53c400, Ricoh, etc. extensions ? */
> -} card;
> +static int base[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
> +module_param_array(base, int, NULL, 0);
> +MODULE_PARM_DESC(base, "base address(es)");
> +
> +static int card[] = { -1, -1, -1, -1, -1, -1, -1, -1 };
> +module_param_array(card, int, NULL, 0);
> +MODULE_PARM_DESC(card, "card type (0=NCR5380, 1=NCR53C400, 2=NCR53C400A, 
> 3=DTC3181E, 4=HP C2502)");
> +
> +MODULE_LICENSE("GPL");
>  
>  #ifndef SCSI_G_NCR5380_MEM
>  /*
> @@ -73,17 +90,8 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
>  }
>  #endif
>  
> -/**
> - *   generic_NCR5380_detect  -   look for NCR5380 controllers
> - *   @tpnt: the scsi template
> - *
> - *   Scan for the present of NCR5380, NCR53C400, NCR53C400A, DTC3181E
> - *   and DTC436(ISAPnP) controllers.
> - *
> - *   Locks: none
> - */
> -
> -static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
> +static int generic_NCR5380_init_one(struct scsi_host_template *tpnt,
> + struct device *pdev, int base, int irq, int board)
>  {
>   unsigned int *ports;
>   u8 *magic = NULL;
> @@ -92,80 +100,29 @@ static int __init generic_NCR5380_detect(struct 
> scsi_host_template *tpnt)
>   int

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

2016-09-28 Thread Finn Thain


Acked-by: Finn Thain 

On Tue, 27 Sep 2016, Ondrej Zary wrote:

> Convert g_NCR5380 to use scsi_add_host instead of scsi_module.c
> Use pnp_driver and isa_driver to manage cards.
> 
> In order to support multiple cards, new module parameter format is
> introduced. The old parameters are kept for compatibility.
> 
> Signed-off-by: Ondrej Zary 
> ---
>  Documentation/scsi/g_NCR5380.txt |   24 ++-
>  drivers/scsi/g_NCR5380.c |  335 
> ++
>  drivers/scsi/g_NCR5380.h |8 -
>  3 files changed, 215 insertions(+), 152 deletions(-)
> 
> diff --git a/Documentation/scsi/g_NCR5380.txt 
> b/Documentation/scsi/g_NCR5380.txt
> index 843cbae..e2c1879 100644
> --- a/Documentation/scsi/g_NCR5380.txt
> +++ b/Documentation/scsi/g_NCR5380.txt
> @@ -28,6 +28,16 @@ time. More info to come in the future.
>  This driver works as a module.
>  When included as a module, parameters can be passed on the insmod/modprobe
>  command line:
> +  irq=xx[,...]   the interrupt(s)
> +  base=xx[,...]  the port or base address(es) (for port or memory 
> mapped, resp.)
> +  card=xx[,...]  card type(s):
> + 0 = NCR5380,
> + 1 = NCR53C400,
> + 2 = NCR53C400A,
> + 3 = Domex Technology Corp 3181E (DTC3181E)
> + 4 = Hewlett Packard C2502
> +
> +These old-style parameters can support only one card:
>ncr_irq=xx   the interrupt
>ncr_addr=xx  the port or base address (for port or memory
> mapped, resp.)
> @@ -36,11 +46,19 @@ command line:
>ncr_53c400a=1 to set up for a NCR53C400A board
>dtc_3181e=1  to set up for a Domex Technology Corp 3181E board
>hp_c2502=1   to set up for a Hewlett Packard C2502 board
> +
>  e.g.
> -modprobe g_NCR5380 ncr_irq=5 ncr_addr=0x350 ncr_5380=1
> +OLD: modprobe g_NCR5380 ncr_irq=5 ncr_addr=0x350 ncr_5380=1
> +NEW: modprobe g_NCR5380 irq=5 base=0x350 card=0
>for a port mapped NCR5380 board or
> -modprobe g_NCR5380 ncr_irq=255 ncr_addr=0xc8000 ncr_53c400=1
> -  for a memory mapped NCR53C400 board with interrupts disabled.
> +
> +OLD: modprobe g_NCR5380 ncr_irq=255 ncr_addr=0xc8000 ncr_53c400=1
> +NEW: modprobe g_NCR5380 irq=255 base=0xc8000 card=1
> +  for a memory mapped NCR53C400 board with interrupts disabled or
> +
> +NEW: modprobe g_NCR5380 irq=0,7 base=0x240,0x300 card=3,4
> +  for two cards: DTC3181 (in non-PnP mode) at 0x240 with no IRQ
> + and HP C2502 at 0x300 with IRQ 7
>  
>  (255 should be specified for no or DMA interrupt, 254 to autoprobe for an 
>   IRQ line if overridden on the command line.)
> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 5162de6..cbf0103 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -30,24 +30,41 @@
>  #include "NCR5380.h"
>  #include 
>  #include 
> -#include 
> +#include 
> +#include 
>  #include 
>  
> +#define MAX_CARDS 8
> +
> +/* old-style parameters for compatibility */
>  static int ncr_irq;
> -static int ncr_dma;
>  static int ncr_addr;
>  static int ncr_5380;
>  static int ncr_53c400;
>  static int ncr_53c400a;
>  static int dtc_3181e;
>  static int hp_c2502;
> +module_param(ncr_irq, int, 0);
> +module_param(ncr_addr, int, 0);
> +module_param(ncr_5380, int, 0);
> +module_param(ncr_53c400, int, 0);
> +module_param(ncr_53c400a, int, 0);
> +module_param(dtc_3181e, int, 0);
> +module_param(hp_c2502, int, 0);
> +
> +static int irq[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
> +module_param_array(irq, int, NULL, 0);
> +MODULE_PARM_DESC(irq, "IRQ number(s)");
>  
> -static struct card {
> - NCR5380_map_type NCR5380_map_name;
> - int irq;
> - int dma;
> - int board;  /* Use NCR53c400, Ricoh, etc. extensions ? */
> -} card;
> +static int base[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
> +module_param_array(base, int, NULL, 0);
> +MODULE_PARM_DESC(base, "base address(es)");
> +
> +static int card[] = { -1, -1, -1, -1, -1, -1, -1, -1 };
> +module_param_array(card, int, NULL, 0);
> +MODULE_PARM_DESC(card, "card type (0=NCR5380, 1=NCR53C400, 2=NCR53C400A, 
> 3=DTC3181E, 4=HP C2502)");
> +
> +MODULE_LICENSE("GPL");
>  
>  #ifndef SCSI_G_NCR5380_MEM
>  /*
> @@ -73,17 +90,8 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
>  }
>  #endif
>  
> -/**
> - *   generic_NCR5380_detect  -   look for NCR5380 controllers
> - *   @tpnt: the scsi template
> - *
> - *   Scan for the present of NCR5380, NCR53C400, NCR53C400A, DTC3181E
> - *   and DTC436(ISAPnP) controllers.
> - *
> - *   Locks: none
> - */
> -
> -static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
> +static int generic_NCR5380_init_one(struct scsi_host_template *tpnt,
> + struct device *pdev, int base, int irq, int board)
>  {
>   unsigned int *ports;
>   u8 *magic = NULL;
> @@ -92,80 +100,29 @@ static int __init generic_NCR5380_detect(struct 
> scsi_host_template *tpnt)
>   int port_idx = -1;
>   unsigned long region_size;
>

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

2016-09-27 Thread Finn Thain


On Tue, 27 Sep 2016, Ondrej Zary wrote:

> On Monday 26 September 2016, Finn Thain wrote:
> 
> [...]
> 
> > > @@ -364,7 +304,7 @@ static int generic_NCR5380_release_resources(struct
> > > Scsi_Host *instance) release_mem_region(instance->base,
> > > hostdata->iomem_size);
> > >   }
> > >  #endif
> > > - return 0;
> > > + scsi_host_put(instance);
> >
> > The sequence of operations here should be the same as the error path
> > above.
> 
> I put scsi_host_put() call at the end because the release_mem_region 
> code (in the MMIO case) dereferences the hostdata pointer. I don't think 
> it's safe to do after scsi_host_put().

It's funny you say that, I already fixed a few of those use-after-free 
bugs in the other 5380 drivers. To avoid the bug, you'd need to use a 
temporary variable, like the fixes I did elsewhere.

Don't worry about changing it, this part of the driver gets revisited in 
my existing patch queue anyway.

> 
> [...]
> 
> > > +static int pnp_registered;
> > > +#endif /* !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) */
> > > +
> > > +static int __init generic_NCR5380_init(void)
> > > +{
> > > + int ret = 0;
> > > +
> > > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > > + ret = pnp_register_driver(_NCR5380_pnp_driver);
> > > + if (!ret)
> > > + pnp_registered = 1;
> > >  #endif
> > > + ret = isa_register_driver(_NCR5380_isa_driver, MAX_CARDS);
> > > + if (!ret)
> > > + isa_registered = 1;
> > > +
> > > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > > + if (pnp_registered)
> > > + ret = 0;
> > > +#endif
> > > + if (isa_registered)
> > > + ret = 0;
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static void __exit generic_NCR5380_exit(void)
> > > +{
> > > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > > + if (pnp_registered)
> > > + pnp_unregister_driver(_NCR5380_pnp_driver);
> > > +#endif
> > > + if (isa_registered)
> > > + isa_unregister_driver(_NCR5380_isa_driver);
> > > +}
> > > +
> >
> > I find that hard to follow. This should be equivalent and simpler:
> >
> > static int __init generic_NCR5380_init(void)
> > {
> > isa_ret = isa_register_driver(_NCR5380_isa_driver, MAX_CARDS);
> > #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > pnp_ret = pnp_register_driver(_NCR5380_pnp_driver);
> > return pnp_ret ? isa_ret : 0;
> > #endif
> > return isa_ret;
> > }
> >
> > static void __exit generic_NCR5380_exit(void)
> > {
> > if (!isa_ret)
> > isa_unregister_driver(_NCR5380_isa_driver);
> > #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > if (!pnp_ret)
> > pnp_unregister_driver(_NCR5380_pnp_driver);
> > #endif
> > }
> 
> Doesn't make it any better, IMHO. Good that it's shorter but not cleaner - 
> especially this:
> > return pnp_ret ? isa_ret : 0;

The problem with that line is that pnp_ret is always discarded, same as in 
your version. I think my version makes that clear, which is what matters 
to me.

> 
> Also looking at the _exit function, meaning of isa_ret and pnp_ret is not 
> obvious there.
> 
> Maybe we should have a module_isa_pnp_driver() macro.
> 
> 

FWIW I would hope that the hypothetical definition of this 
`module_isa_pnp_driver' macro would have the same properties as my 
version: shorter, uses no temporaries and uses one less #ifdef, and is 
generally easier for me to read.

--

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

2016-09-27 Thread Finn Thain


On Tue, 27 Sep 2016, Ondrej Zary wrote:

> On Monday 26 September 2016, Finn Thain wrote:
> 
> [...]
> 
> > > @@ -364,7 +304,7 @@ static int generic_NCR5380_release_resources(struct
> > > Scsi_Host *instance) release_mem_region(instance->base,
> > > hostdata->iomem_size);
> > >   }
> > >  #endif
> > > - return 0;
> > > + scsi_host_put(instance);
> >
> > The sequence of operations here should be the same as the error path
> > above.
> 
> I put scsi_host_put() call at the end because the release_mem_region 
> code (in the MMIO case) dereferences the hostdata pointer. I don't think 
> it's safe to do after scsi_host_put().

It's funny you say that, I already fixed a few of those use-after-free 
bugs in the other 5380 drivers. To avoid the bug, you'd need to use a 
temporary variable, like the fixes I did elsewhere.

Don't worry about changing it, this part of the driver gets revisited in 
my existing patch queue anyway.

> 
> [...]
> 
> > > +static int pnp_registered;
> > > +#endif /* !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) */
> > > +
> > > +static int __init generic_NCR5380_init(void)
> > > +{
> > > + int ret = 0;
> > > +
> > > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > > + ret = pnp_register_driver(_NCR5380_pnp_driver);
> > > + if (!ret)
> > > + pnp_registered = 1;
> > >  #endif
> > > + ret = isa_register_driver(_NCR5380_isa_driver, MAX_CARDS);
> > > + if (!ret)
> > > + isa_registered = 1;
> > > +
> > > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > > + if (pnp_registered)
> > > + ret = 0;
> > > +#endif
> > > + if (isa_registered)
> > > + ret = 0;
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static void __exit generic_NCR5380_exit(void)
> > > +{
> > > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > > + if (pnp_registered)
> > > + pnp_unregister_driver(_NCR5380_pnp_driver);
> > > +#endif
> > > + if (isa_registered)
> > > + isa_unregister_driver(_NCR5380_isa_driver);
> > > +}
> > > +
> >
> > I find that hard to follow. This should be equivalent and simpler:
> >
> > static int __init generic_NCR5380_init(void)
> > {
> > isa_ret = isa_register_driver(_NCR5380_isa_driver, MAX_CARDS);
> > #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > pnp_ret = pnp_register_driver(_NCR5380_pnp_driver);
> > return pnp_ret ? isa_ret : 0;
> > #endif
> > return isa_ret;
> > }
> >
> > static void __exit generic_NCR5380_exit(void)
> > {
> > if (!isa_ret)
> > isa_unregister_driver(_NCR5380_isa_driver);
> > #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > if (!pnp_ret)
> > pnp_unregister_driver(_NCR5380_pnp_driver);
> > #endif
> > }
> 
> Doesn't make it any better, IMHO. Good that it's shorter but not cleaner - 
> especially this:
> > return pnp_ret ? isa_ret : 0;

The problem with that line is that pnp_ret is always discarded, same as in 
your version. I think my version makes that clear, which is what matters 
to me.

> 
> Also looking at the _exit function, meaning of isa_ret and pnp_ret is not 
> obvious there.
> 
> Maybe we should have a module_isa_pnp_driver() macro.
> 
> 

FWIW I would hope that the hypothetical definition of this 
`module_isa_pnp_driver' macro would have the same properties as my 
version: shorter, uses no temporaries and uses one less #ifdef, and is 
generally easier for me to read.

--

[PATCH 3/3] g_NCR5380: Stop using scsi_module.c

2016-09-27 Thread Ondrej Zary

Convert g_NCR5380 to use scsi_add_host instead of scsi_module.c
Use pnp_driver and isa_driver to manage cards.

In order to support multiple cards, new module parameter format is
introduced. The old parameters are kept for compatibility.

Signed-off-by: Ondrej Zary 
---
 Documentation/scsi/g_NCR5380.txt |   24 ++-
 drivers/scsi/g_NCR5380.c |  335 ++
 drivers/scsi/g_NCR5380.h |8 -
 3 files changed, 215 insertions(+), 152 deletions(-)

diff --git a/Documentation/scsi/g_NCR5380.txt b/Documentation/scsi/g_NCR5380.txt
index 843cbae..e2c1879 100644
--- a/Documentation/scsi/g_NCR5380.txt
+++ b/Documentation/scsi/g_NCR5380.txt
@@ -28,6 +28,16 @@ time. More info to come in the future.
 This driver works as a module.
 When included as a module, parameters can be passed on the insmod/modprobe
 command line:
+  irq=xx[,...] the interrupt(s)
+  base=xx[,...]the port or base address(es) (for port or memory 
mapped, resp.)
+  card=xx[,...]card type(s):
+   0 = NCR5380,
+   1 = NCR53C400,
+   2 = NCR53C400A,
+   3 = Domex Technology Corp 3181E (DTC3181E)
+   4 = Hewlett Packard C2502
+
+These old-style parameters can support only one card:
   ncr_irq=xx   the interrupt
   ncr_addr=xx  the port or base address (for port or memory
mapped, resp.)
@@ -36,11 +46,19 @@ command line:
   ncr_53c400a=1 to set up for a NCR53C400A board
   dtc_3181e=1  to set up for a Domex Technology Corp 3181E board
   hp_c2502=1   to set up for a Hewlett Packard C2502 board
+
 e.g.
-modprobe g_NCR5380 ncr_irq=5 ncr_addr=0x350 ncr_5380=1
+OLD: modprobe g_NCR5380 ncr_irq=5 ncr_addr=0x350 ncr_5380=1
+NEW: modprobe g_NCR5380 irq=5 base=0x350 card=0
   for a port mapped NCR5380 board or
-modprobe g_NCR5380 ncr_irq=255 ncr_addr=0xc8000 ncr_53c400=1
-  for a memory mapped NCR53C400 board with interrupts disabled.
+
+OLD: modprobe g_NCR5380 ncr_irq=255 ncr_addr=0xc8000 ncr_53c400=1
+NEW: modprobe g_NCR5380 irq=255 base=0xc8000 card=1
+  for a memory mapped NCR53C400 board with interrupts disabled or
+
+NEW: modprobe g_NCR5380 irq=0,7 base=0x240,0x300 card=3,4
+  for two cards: DTC3181 (in non-PnP mode) at 0x240 with no IRQ
+ and HP C2502 at 0x300 with IRQ 7
 
 (255 should be specified for no or DMA interrupt, 254 to autoprobe for an 
  IRQ line if overridden on the command line.)
diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 5162de6..cbf0103 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -30,24 +30,41 @@
 #include "NCR5380.h"
 #include 
 #include 
-#include 
+#include 
+#include 
 #include 
 
+#define MAX_CARDS 8
+
+/* old-style parameters for compatibility */
 static int ncr_irq;
-static int ncr_dma;
 static int ncr_addr;
 static int ncr_5380;
 static int ncr_53c400;
 static int ncr_53c400a;
 static int dtc_3181e;
 static int hp_c2502;
+module_param(ncr_irq, int, 0);
+module_param(ncr_addr, int, 0);
+module_param(ncr_5380, int, 0);
+module_param(ncr_53c400, int, 0);
+module_param(ncr_53c400a, int, 0);
+module_param(dtc_3181e, int, 0);
+module_param(hp_c2502, int, 0);
+
+static int irq[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+module_param_array(irq, int, NULL, 0);
+MODULE_PARM_DESC(irq, "IRQ number(s)");
 
-static struct card {
-   NCR5380_map_type NCR5380_map_name;
-   int irq;
-   int dma;
-   int board;  /* Use NCR53c400, Ricoh, etc. extensions ? */
-} card;
+static int base[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+module_param_array(base, int, NULL, 0);
+MODULE_PARM_DESC(base, "base address(es)");
+
+static int card[] = { -1, -1, -1, -1, -1, -1, -1, -1 };
+module_param_array(card, int, NULL, 0);
+MODULE_PARM_DESC(card, "card type (0=NCR5380, 1=NCR53C400, 2=NCR53C400A, 
3=DTC3181E, 4=HP C2502)");
+
+MODULE_LICENSE("GPL");
 
 #ifndef SCSI_G_NCR5380_MEM
 /*
@@ -73,17 +90,8 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
 }
 #endif
 
-/**
- * generic_NCR5380_detect  -   look for NCR5380 controllers
- * @tpnt: the scsi template
- *
- * Scan for the present of NCR5380, NCR53C400, NCR53C400A, DTC3181E
- * and DTC436(ISAPnP) controllers.
- *
- * Locks: none
- */
-
-static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
+static int generic_NCR5380_init_one(struct scsi_host_template *tpnt,
+   struct device *pdev, int base, int irq, int board)
 {
unsigned int *ports;
u8 *magic = NULL;
@@ -92,80 +100,29 @@ static int __init generic_NCR5380_detect(struct 
scsi_host_template *tpnt)
int port_idx = -1;
unsigned long region_size;
 #endif
-   static unsigned int __initdata ncr_53c400a_ports[] = {
+   static unsigned int ncr_53c400a_ports[] = {
0x280, 0x290, 0x300, 0x310, 0x330, 0x340, 0x348, 0x350, 0
};
-   static unsigned int __initdata dtc_3181e_ports[] = {
+   static

[PATCH 3/3] g_NCR5380: Stop using scsi_module.c

2016-09-27 Thread Ondrej Zary

Convert g_NCR5380 to use scsi_add_host instead of scsi_module.c
Use pnp_driver and isa_driver to manage cards.

In order to support multiple cards, new module parameter format is
introduced. The old parameters are kept for compatibility.

Signed-off-by: Ondrej Zary 
---
 Documentation/scsi/g_NCR5380.txt |   24 ++-
 drivers/scsi/g_NCR5380.c |  335 ++
 drivers/scsi/g_NCR5380.h |8 -
 3 files changed, 215 insertions(+), 152 deletions(-)

diff --git a/Documentation/scsi/g_NCR5380.txt b/Documentation/scsi/g_NCR5380.txt
index 843cbae..e2c1879 100644
--- a/Documentation/scsi/g_NCR5380.txt
+++ b/Documentation/scsi/g_NCR5380.txt
@@ -28,6 +28,16 @@ time. More info to come in the future.
 This driver works as a module.
 When included as a module, parameters can be passed on the insmod/modprobe
 command line:
+  irq=xx[,...] the interrupt(s)
+  base=xx[,...]the port or base address(es) (for port or memory 
mapped, resp.)
+  card=xx[,...]card type(s):
+   0 = NCR5380,
+   1 = NCR53C400,
+   2 = NCR53C400A,
+   3 = Domex Technology Corp 3181E (DTC3181E)
+   4 = Hewlett Packard C2502
+
+These old-style parameters can support only one card:
   ncr_irq=xx   the interrupt
   ncr_addr=xx  the port or base address (for port or memory
mapped, resp.)
@@ -36,11 +46,19 @@ command line:
   ncr_53c400a=1 to set up for a NCR53C400A board
   dtc_3181e=1  to set up for a Domex Technology Corp 3181E board
   hp_c2502=1   to set up for a Hewlett Packard C2502 board
+
 e.g.
-modprobe g_NCR5380 ncr_irq=5 ncr_addr=0x350 ncr_5380=1
+OLD: modprobe g_NCR5380 ncr_irq=5 ncr_addr=0x350 ncr_5380=1
+NEW: modprobe g_NCR5380 irq=5 base=0x350 card=0
   for a port mapped NCR5380 board or
-modprobe g_NCR5380 ncr_irq=255 ncr_addr=0xc8000 ncr_53c400=1
-  for a memory mapped NCR53C400 board with interrupts disabled.
+
+OLD: modprobe g_NCR5380 ncr_irq=255 ncr_addr=0xc8000 ncr_53c400=1
+NEW: modprobe g_NCR5380 irq=255 base=0xc8000 card=1
+  for a memory mapped NCR53C400 board with interrupts disabled or
+
+NEW: modprobe g_NCR5380 irq=0,7 base=0x240,0x300 card=3,4
+  for two cards: DTC3181 (in non-PnP mode) at 0x240 with no IRQ
+ and HP C2502 at 0x300 with IRQ 7
 
 (255 should be specified for no or DMA interrupt, 254 to autoprobe for an 
  IRQ line if overridden on the command line.)
diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 5162de6..cbf0103 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -30,24 +30,41 @@
 #include "NCR5380.h"
 #include 
 #include 
-#include 
+#include 
+#include 
 #include 
 
+#define MAX_CARDS 8
+
+/* old-style parameters for compatibility */
 static int ncr_irq;
-static int ncr_dma;
 static int ncr_addr;
 static int ncr_5380;
 static int ncr_53c400;
 static int ncr_53c400a;
 static int dtc_3181e;
 static int hp_c2502;
+module_param(ncr_irq, int, 0);
+module_param(ncr_addr, int, 0);
+module_param(ncr_5380, int, 0);
+module_param(ncr_53c400, int, 0);
+module_param(ncr_53c400a, int, 0);
+module_param(dtc_3181e, int, 0);
+module_param(hp_c2502, int, 0);
+
+static int irq[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+module_param_array(irq, int, NULL, 0);
+MODULE_PARM_DESC(irq, "IRQ number(s)");
 
-static struct card {
-   NCR5380_map_type NCR5380_map_name;
-   int irq;
-   int dma;
-   int board;  /* Use NCR53c400, Ricoh, etc. extensions ? */
-} card;
+static int base[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+module_param_array(base, int, NULL, 0);
+MODULE_PARM_DESC(base, "base address(es)");
+
+static int card[] = { -1, -1, -1, -1, -1, -1, -1, -1 };
+module_param_array(card, int, NULL, 0);
+MODULE_PARM_DESC(card, "card type (0=NCR5380, 1=NCR53C400, 2=NCR53C400A, 
3=DTC3181E, 4=HP C2502)");
+
+MODULE_LICENSE("GPL");
 
 #ifndef SCSI_G_NCR5380_MEM
 /*
@@ -73,17 +90,8 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
 }
 #endif
 
-/**
- * generic_NCR5380_detect  -   look for NCR5380 controllers
- * @tpnt: the scsi template
- *
- * Scan for the present of NCR5380, NCR53C400, NCR53C400A, DTC3181E
- * and DTC436(ISAPnP) controllers.
- *
- * Locks: none
- */
-
-static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
+static int generic_NCR5380_init_one(struct scsi_host_template *tpnt,
+   struct device *pdev, int base, int irq, int board)
 {
unsigned int *ports;
u8 *magic = NULL;
@@ -92,80 +100,29 @@ static int __init generic_NCR5380_detect(struct 
scsi_host_template *tpnt)
int port_idx = -1;
unsigned long region_size;
 #endif
-   static unsigned int __initdata ncr_53c400a_ports[] = {
+   static unsigned int ncr_53c400a_ports[] = {
0x280, 0x290, 0x300, 0x310, 0x330, 0x340, 0x348, 0x350, 0
};
-   static unsigned int __initdata dtc_3181e_ports[] = {
+   static unsigned int dtc_3181e_ports[]

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

2016-09-27 Thread Ondrej Zary

On Monday 26 September 2016, Finn Thain wrote:

[...]

> Instead of this:
> > +   if (scsi_add_host(instance, pdev)) {
> > +   free_irq(irq, instance);
> > +   goto out_unregister;
> > +   }
> > +   scsi_scan_host(instance);
> > +   return instance;
>
> please use the following (note the missing NCR5380_exit() call):
>
> + if (scsi_add_host(instance, pdev))
> + goto out_free_irq;
> + scsi_scan_host(instance);
> + return(instance);
> +
> +out_free_irq:
> + if (instance->irq != NO_IRQ)
> + free_irq(instance->irq, instance);
> + NCR5380_exit(instance);
>
> >  out_unregister:
> > -   scsi_unregister(instance);
> > +   scsi_host_put(instance);
> >  out_release:
> >  #ifndef SCSI_G_NCR5380_MEM
> > -   release_region(card.NCR5380_map_name, region_size);
> > +   release_region(base, region_size);
> >  #else
> > iounmap(iomem);
> > release_mem_region(base, iomem_size);
> >  #endif
> > -   return 0;
> > +   return NULL;
> >  }
> >
> > -/**
> > - * generic_NCR5380_release_resources   -   free resources
> > - * @instance: host adapter to clean up
> > - *
> > - * Free the generic interface resources from this adapter.
> > - *
> > - * Locks: none
> > - */
> > -
> > -static int generic_NCR5380_release_resources(struct Scsi_Host *instance)
> > +static void generic_NCR5380_release_resources(struct Scsi_Host
> > *instance) {
> > +   scsi_remove_host(instance);
> > if (instance->irq != NO_IRQ)
> > free_irq(instance->irq, instance);
> > NCR5380_exit(instance);
> > @@ -364,7 +304,7 @@ static int generic_NCR5380_release_resources(struct
> > Scsi_Host *instance) release_mem_region(instance->base,
> > hostdata->iomem_size);
> > }
> >  #endif
> > -   return 0;
> > +   scsi_host_put(instance);
>
> The sequence of operations here should be the same as the error path
> above.

I put scsi_host_put() call at the end because the release_mem_region code (in 
the MMIO case) dereferences the hostdata pointer. I don't think it's safe to 
do after scsi_host_put().

[...]

> > +static int pnp_registered;
> > +#endif /* !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) */
> > +
> > +static int __init generic_NCR5380_init(void)
> > +{
> > +   int ret = 0;
> > +
> > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > +   ret = pnp_register_driver(_NCR5380_pnp_driver);
> > +   if (!ret)
> > +   pnp_registered = 1;
> >  #endif
> > +   ret = isa_register_driver(_NCR5380_isa_driver, MAX_CARDS);
> > +   if (!ret)
> > +   isa_registered = 1;
> > +
> > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > +   if (pnp_registered)
> > +   ret = 0;
> > +#endif
> > +   if (isa_registered)
> > +   ret = 0;
> > +
> > +   return ret;
> > +}
> > +
> > +static void __exit generic_NCR5380_exit(void)
> > +{
> > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > +   if (pnp_registered)
> > +   pnp_unregister_driver(_NCR5380_pnp_driver);
> > +#endif
> > +   if (isa_registered)
> > +   isa_unregister_driver(_NCR5380_isa_driver);
> > +}
> > +
>
> I find that hard to follow. This should be equivalent and simpler:
>
> static int __init generic_NCR5380_init(void)
> {
>   isa_ret = isa_register_driver(_NCR5380_isa_driver, MAX_CARDS);
> #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
>   pnp_ret = pnp_register_driver(_NCR5380_pnp_driver);
>   return pnp_ret ? isa_ret : 0;
> #endif
>   return isa_ret;
> }
>
> static void __exit generic_NCR5380_exit(void)
> {
>   if (!isa_ret)
>   isa_unregister_driver(_NCR5380_isa_driver);
> #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
>   if (!pnp_ret)
>   pnp_unregister_driver(_NCR5380_pnp_driver);
> #endif
> }

Doesn't make it any better, IMHO. Good that it's shorter but not cleaner - 
especially this:
>   return pnp_ret ? isa_ret : 0;

Also looking at the _exit function, meaning of isa_ret and pnp_ret is not 
obvious there.

Maybe we should have a module_isa_pnp_driver() macro.

-- 
Ondrej Zary

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

2016-09-27 Thread Ondrej Zary

On Monday 26 September 2016, Finn Thain wrote:

[...]

> Instead of this:
> > +   if (scsi_add_host(instance, pdev)) {
> > +   free_irq(irq, instance);
> > +   goto out_unregister;
> > +   }
> > +   scsi_scan_host(instance);
> > +   return instance;
>
> please use the following (note the missing NCR5380_exit() call):
>
> + if (scsi_add_host(instance, pdev))
> + goto out_free_irq;
> + scsi_scan_host(instance);
> + return(instance);
> +
> +out_free_irq:
> + if (instance->irq != NO_IRQ)
> + free_irq(instance->irq, instance);
> + NCR5380_exit(instance);
>
> >  out_unregister:
> > -   scsi_unregister(instance);
> > +   scsi_host_put(instance);
> >  out_release:
> >  #ifndef SCSI_G_NCR5380_MEM
> > -   release_region(card.NCR5380_map_name, region_size);
> > +   release_region(base, region_size);
> >  #else
> > iounmap(iomem);
> > release_mem_region(base, iomem_size);
> >  #endif
> > -   return 0;
> > +   return NULL;
> >  }
> >
> > -/**
> > - * generic_NCR5380_release_resources   -   free resources
> > - * @instance: host adapter to clean up
> > - *
> > - * Free the generic interface resources from this adapter.
> > - *
> > - * Locks: none
> > - */
> > -
> > -static int generic_NCR5380_release_resources(struct Scsi_Host *instance)
> > +static void generic_NCR5380_release_resources(struct Scsi_Host
> > *instance) {
> > +   scsi_remove_host(instance);
> > if (instance->irq != NO_IRQ)
> > free_irq(instance->irq, instance);
> > NCR5380_exit(instance);
> > @@ -364,7 +304,7 @@ static int generic_NCR5380_release_resources(struct
> > Scsi_Host *instance) release_mem_region(instance->base,
> > hostdata->iomem_size);
> > }
> >  #endif
> > -   return 0;
> > +   scsi_host_put(instance);
>
> The sequence of operations here should be the same as the error path
> above.

I put scsi_host_put() call at the end because the release_mem_region code (in 
the MMIO case) dereferences the hostdata pointer. I don't think it's safe to 
do after scsi_host_put().

[...]

> > +static int pnp_registered;
> > +#endif /* !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) */
> > +
> > +static int __init generic_NCR5380_init(void)
> > +{
> > +   int ret = 0;
> > +
> > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > +   ret = pnp_register_driver(_NCR5380_pnp_driver);
> > +   if (!ret)
> > +   pnp_registered = 1;
> >  #endif
> > +   ret = isa_register_driver(_NCR5380_isa_driver, MAX_CARDS);
> > +   if (!ret)
> > +   isa_registered = 1;
> > +
> > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > +   if (pnp_registered)
> > +   ret = 0;
> > +#endif
> > +   if (isa_registered)
> > +   ret = 0;
> > +
> > +   return ret;
> > +}
> > +
> > +static void __exit generic_NCR5380_exit(void)
> > +{
> > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > +   if (pnp_registered)
> > +   pnp_unregister_driver(_NCR5380_pnp_driver);
> > +#endif
> > +   if (isa_registered)
> > +   isa_unregister_driver(_NCR5380_isa_driver);
> > +}
> > +
>
> I find that hard to follow. This should be equivalent and simpler:
>
> static int __init generic_NCR5380_init(void)
> {
>   isa_ret = isa_register_driver(_NCR5380_isa_driver, MAX_CARDS);
> #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
>   pnp_ret = pnp_register_driver(_NCR5380_pnp_driver);
>   return pnp_ret ? isa_ret : 0;
> #endif
>   return isa_ret;
> }
>
> static void __exit generic_NCR5380_exit(void)
> {
>   if (!isa_ret)
>   isa_unregister_driver(_NCR5380_isa_driver);
> #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
>   if (!pnp_ret)
>   pnp_unregister_driver(_NCR5380_pnp_driver);
> #endif
> }

Doesn't make it any better, IMHO. Good that it's shorter but not cleaner - 
especially this:
>   return pnp_ret ? isa_ret : 0;

Also looking at the _exit function, meaning of isa_ret and pnp_ret is not 
obvious there.

Maybe we should have a module_isa_pnp_driver() macro.

-- 
Ondrej Zary

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

2016-09-25 Thread Finn Thain


On Sat, 24 Sep 2016, Ondrej Zary wrote:

> Convert g_NCR5380 to use scsi_add_host instead of scsi_module.c
> Use pnp_driver and isa_driver to manage cards.
> 
> Signed-off-by: Ondrej Zary 
> ---
>  drivers/scsi/g_NCR5380.c |  310 
> +-
>  drivers/scsi/g_NCR5380.h |8 --
>  2 files changed, 169 insertions(+), 149 deletions(-)
> 
> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 5162de6..6cf6b00 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -30,24 +30,25 @@
>  #include "NCR5380.h"
>  #include 
>  #include 
> -#include 
> +#include 
> +#include 
>  #include 
>  
> -static int ncr_irq;
> -static int ncr_dma;
> -static int ncr_addr;
> -static int ncr_5380;
> -static int ncr_53c400;
> -static int ncr_53c400a;
> -static int dtc_3181e;
> -static int hp_c2502;
> -
> -static struct card {
> - NCR5380_map_type NCR5380_map_name;
> - int irq;
> - int dma;
> - int board;  /* Use NCR53c400, Ricoh, etc. extensions ? */
> -} card;
> +#define MAX_CARDS 8
> +
> +static int irq[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
> +module_param_array(irq, int, NULL, 0);
> +MODULE_PARM_DESC(irq, "IRQ number(s)");
> +
> +static int base[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
> +module_param_array(base, int, NULL, 0);
> +MODULE_PARM_DESC(base, "base address(es)");
> +
> +static int card[] = { -1, -1, -1, -1, -1, -1, -1, -1 };
> +module_param_array(card, int, NULL, 0);
> +MODULE_PARM_DESC(card, "card type (0=NCR5380, 1=NCR53C400, 2=NCR53C400A, 
> 3=DTC3181E, 4=HP C2502)");
> +
> +MODULE_LICENSE("GPL");


If you are going to change all of the module parameters and break every 
user's setup, at least explain what changed and why in the commit log and 
update the documentation in the kernel tree.

If you really want to help those users (most of whom gain nothing from 
this patch) you could also update the documentation elsewhere:

http://www.sane-project.org/man/sane-scsi.5.html
http://www.xsane.org/rauch-domain/sane-umax/sane-umax-config-doc.html
http://www.meier-geinitz.de/sane/mustek-backend/

I'm not in favour of this patch, but Christoph liked it so I'll leave it 
at that.

>  
>  #ifndef SCSI_G_NCR5380_MEM
>  /*
> @@ -73,17 +74,9 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
>  }
>  #endif
>  
> -/**
> - *   generic_NCR5380_detect  -   look for NCR5380 controllers
> - *   @tpnt: the scsi template
> - *
> - *   Scan for the present of NCR5380, NCR53C400, NCR53C400A, DTC3181E
> - *   and DTC436(ISAPnP) controllers.
> - *
> - *   Locks: none
> - */
> -
> -static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
> +static struct Scsi_Host *generic_NCR5380_hw_init(
> + struct scsi_host_template *tpnt, struct device *pdev,
> + int base, int irq, int board)

This allocates and returns a Scsi_Host so the name is misleading.

But the name isn't the only problem; the real problem has already been 
pointed out by Christoph: the caller needs the error code to be returned 
not the Scsi_Host pointer.


>  {
>   unsigned int *ports;
>   u8 *magic = NULL;
> @@ -108,64 +101,13 @@ static int __init generic_NCR5380_detect(struct 
> scsi_host_template *tpnt)
>   struct Scsi_Host *instance;
>   struct NCR5380_hostdata *hostdata;
>  #ifdef SCSI_G_NCR5380_MEM
> - unsigned long base;
>   void __iomem *iomem;
>   resource_size_t iomem_size;
>  #endif
>  
> - if (ncr_irq)
> - card.irq = ncr_irq;
> - if (ncr_dma)
> - card.dma = ncr_dma;
> - if (ncr_addr)
> - card.NCR5380_map_name = (NCR5380_map_type) ncr_addr;
> - if (ncr_5380)
> - card.board = BOARD_NCR5380;
> - else if (ncr_53c400)
> - card.board = BOARD_NCR53C400;
> - else if (ncr_53c400a)
> - card.board = BOARD_NCR53C400A;
> - else if (dtc_3181e)
> - card.board = BOARD_DTC3181E;
> - else if (hp_c2502)
> - card.board = BOARD_HP_C2502;
> -#ifndef SCSI_G_NCR5380_MEM
> - if (isapnp_present()) {
> - struct pnp_dev *dev = NULL;
> - while ((dev = pnp_find_dev(NULL, ISAPNP_VENDOR('D', 'T', 'C'), 
> ISAPNP_FUNCTION(0x436e), dev))) {
> - if (pnp_device_attach(dev) < 0)
> - continue;
> - if (pnp_activate_dev(dev) < 0) {
> - printk(KERN_ERR "dtc436e probe: activate 
> failed\n");
> - pnp_device_detach(dev);
> - continue;
> - }
> - if (!pnp_port_valid(dev, 0)) {
> - printk(KERN_ERR "dtc436e probe: no valid 
> port\n");
> - pnp_device_detach(dev);
> - continue;
> - }
> - if (pnp_irq_valid(dev, 0))
> -

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

2016-09-25 Thread Finn Thain


On Sat, 24 Sep 2016, Ondrej Zary wrote:

> Convert g_NCR5380 to use scsi_add_host instead of scsi_module.c
> Use pnp_driver and isa_driver to manage cards.
> 
> Signed-off-by: Ondrej Zary 
> ---
>  drivers/scsi/g_NCR5380.c |  310 
> +-
>  drivers/scsi/g_NCR5380.h |8 --
>  2 files changed, 169 insertions(+), 149 deletions(-)
> 
> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 5162de6..6cf6b00 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -30,24 +30,25 @@
>  #include "NCR5380.h"
>  #include 
>  #include 
> -#include 
> +#include 
> +#include 
>  #include 
>  
> -static int ncr_irq;
> -static int ncr_dma;
> -static int ncr_addr;
> -static int ncr_5380;
> -static int ncr_53c400;
> -static int ncr_53c400a;
> -static int dtc_3181e;
> -static int hp_c2502;
> -
> -static struct card {
> - NCR5380_map_type NCR5380_map_name;
> - int irq;
> - int dma;
> - int board;  /* Use NCR53c400, Ricoh, etc. extensions ? */
> -} card;
> +#define MAX_CARDS 8
> +
> +static int irq[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
> +module_param_array(irq, int, NULL, 0);
> +MODULE_PARM_DESC(irq, "IRQ number(s)");
> +
> +static int base[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
> +module_param_array(base, int, NULL, 0);
> +MODULE_PARM_DESC(base, "base address(es)");
> +
> +static int card[] = { -1, -1, -1, -1, -1, -1, -1, -1 };
> +module_param_array(card, int, NULL, 0);
> +MODULE_PARM_DESC(card, "card type (0=NCR5380, 1=NCR53C400, 2=NCR53C400A, 
> 3=DTC3181E, 4=HP C2502)");
> +
> +MODULE_LICENSE("GPL");


If you are going to change all of the module parameters and break every 
user's setup, at least explain what changed and why in the commit log and 
update the documentation in the kernel tree.

If you really want to help those users (most of whom gain nothing from 
this patch) you could also update the documentation elsewhere:

http://www.sane-project.org/man/sane-scsi.5.html
http://www.xsane.org/rauch-domain/sane-umax/sane-umax-config-doc.html
http://www.meier-geinitz.de/sane/mustek-backend/

I'm not in favour of this patch, but Christoph liked it so I'll leave it 
at that.

>  
>  #ifndef SCSI_G_NCR5380_MEM
>  /*
> @@ -73,17 +74,9 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
>  }
>  #endif
>  
> -/**
> - *   generic_NCR5380_detect  -   look for NCR5380 controllers
> - *   @tpnt: the scsi template
> - *
> - *   Scan for the present of NCR5380, NCR53C400, NCR53C400A, DTC3181E
> - *   and DTC436(ISAPnP) controllers.
> - *
> - *   Locks: none
> - */
> -
> -static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
> +static struct Scsi_Host *generic_NCR5380_hw_init(
> + struct scsi_host_template *tpnt, struct device *pdev,
> + int base, int irq, int board)

This allocates and returns a Scsi_Host so the name is misleading.

But the name isn't the only problem; the real problem has already been 
pointed out by Christoph: the caller needs the error code to be returned 
not the Scsi_Host pointer.


>  {
>   unsigned int *ports;
>   u8 *magic = NULL;
> @@ -108,64 +101,13 @@ static int __init generic_NCR5380_detect(struct 
> scsi_host_template *tpnt)
>   struct Scsi_Host *instance;
>   struct NCR5380_hostdata *hostdata;
>  #ifdef SCSI_G_NCR5380_MEM
> - unsigned long base;
>   void __iomem *iomem;
>   resource_size_t iomem_size;
>  #endif
>  
> - if (ncr_irq)
> - card.irq = ncr_irq;
> - if (ncr_dma)
> - card.dma = ncr_dma;
> - if (ncr_addr)
> - card.NCR5380_map_name = (NCR5380_map_type) ncr_addr;
> - if (ncr_5380)
> - card.board = BOARD_NCR5380;
> - else if (ncr_53c400)
> - card.board = BOARD_NCR53C400;
> - else if (ncr_53c400a)
> - card.board = BOARD_NCR53C400A;
> - else if (dtc_3181e)
> - card.board = BOARD_DTC3181E;
> - else if (hp_c2502)
> - card.board = BOARD_HP_C2502;
> -#ifndef SCSI_G_NCR5380_MEM
> - if (isapnp_present()) {
> - struct pnp_dev *dev = NULL;
> - while ((dev = pnp_find_dev(NULL, ISAPNP_VENDOR('D', 'T', 'C'), 
> ISAPNP_FUNCTION(0x436e), dev))) {
> - if (pnp_device_attach(dev) < 0)
> - continue;
> - if (pnp_activate_dev(dev) < 0) {
> - printk(KERN_ERR "dtc436e probe: activate 
> failed\n");
> - pnp_device_detach(dev);
> - continue;
> - }
> - if (!pnp_port_valid(dev, 0)) {
> - printk(KERN_ERR "dtc436e probe: no valid 
> port\n");
> - pnp_device_detach(dev);
> - continue;
> - }
> - if (pnp_irq_valid(dev, 0))
> - card.irq =

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

2016-09-25 Thread Christoph Hellwig

> +static int generic_NCR5380_isa_match(struct device *pdev, unsigned int ndev)
> +{
> + struct Scsi_Host *sh = NULL;
> +
> + sh = generic_NCR5380_hw_init(_template, pdev, base[ndev],
> +  irq[ndev], card[ndev]);
> + if (!sh && base[ndev])
> + printk(KERN_WARNING "Card not found at address 0x%03x\n",
> + base[ndev]);
> + if (!sh)
> + return 0;
>  
> + dev_set_drvdata(pdev, sh);

Any reason not to move the dev_set_drvdata into generic_NCR5380_hw_init?
That would also allow to properly propagate the error down to the
caller, which would be useful for the PNP case.

Otherwise this look great to me.

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

2016-09-25 Thread Christoph Hellwig

> +static int generic_NCR5380_isa_match(struct device *pdev, unsigned int ndev)
> +{
> + struct Scsi_Host *sh = NULL;
> +
> + sh = generic_NCR5380_hw_init(_template, pdev, base[ndev],
> +  irq[ndev], card[ndev]);
> + if (!sh && base[ndev])
> + printk(KERN_WARNING "Card not found at address 0x%03x\n",
> + base[ndev]);
> + if (!sh)
> + return 0;
>  
> + dev_set_drvdata(pdev, sh);

Any reason not to move the dev_set_drvdata into generic_NCR5380_hw_init?
That would also allow to properly propagate the error down to the
caller, which would be useful for the PNP case.

Otherwise this look great to me.

[PATCH 3/3] g_NCR5380: Stop using scsi_module.c

2016-09-24 Thread Ondrej Zary

Convert g_NCR5380 to use scsi_add_host instead of scsi_module.c
Use pnp_driver and isa_driver to manage cards.

Signed-off-by: Ondrej Zary 
---
 drivers/scsi/g_NCR5380.c |  310 +-
 drivers/scsi/g_NCR5380.h |8 --
 2 files changed, 169 insertions(+), 149 deletions(-)

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 5162de6..6cf6b00 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -30,24 +30,25 @@
 #include "NCR5380.h"
 #include 
 #include 
-#include 
+#include 
+#include 
 #include 
 
-static int ncr_irq;
-static int ncr_dma;
-static int ncr_addr;
-static int ncr_5380;
-static int ncr_53c400;
-static int ncr_53c400a;
-static int dtc_3181e;
-static int hp_c2502;
-
-static struct card {
-   NCR5380_map_type NCR5380_map_name;
-   int irq;
-   int dma;
-   int board;  /* Use NCR53c400, Ricoh, etc. extensions ? */
-} card;
+#define MAX_CARDS 8
+
+static int irq[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+module_param_array(irq, int, NULL, 0);
+MODULE_PARM_DESC(irq, "IRQ number(s)");
+
+static int base[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+module_param_array(base, int, NULL, 0);
+MODULE_PARM_DESC(base, "base address(es)");
+
+static int card[] = { -1, -1, -1, -1, -1, -1, -1, -1 };
+module_param_array(card, int, NULL, 0);
+MODULE_PARM_DESC(card, "card type (0=NCR5380, 1=NCR53C400, 2=NCR53C400A, 
3=DTC3181E, 4=HP C2502)");
+
+MODULE_LICENSE("GPL");
 
 #ifndef SCSI_G_NCR5380_MEM
 /*
@@ -73,17 +74,9 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
 }
 #endif
 
-/**
- * generic_NCR5380_detect  -   look for NCR5380 controllers
- * @tpnt: the scsi template
- *
- * Scan for the present of NCR5380, NCR53C400, NCR53C400A, DTC3181E
- * and DTC436(ISAPnP) controllers.
- *
- * Locks: none
- */
-
-static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
+static struct Scsi_Host *generic_NCR5380_hw_init(
+   struct scsi_host_template *tpnt, struct device *pdev,
+   int base, int irq, int board)
 {
unsigned int *ports;
u8 *magic = NULL;
@@ -108,64 +101,13 @@ static int __init generic_NCR5380_detect(struct 
scsi_host_template *tpnt)
struct Scsi_Host *instance;
struct NCR5380_hostdata *hostdata;
 #ifdef SCSI_G_NCR5380_MEM
-   unsigned long base;
void __iomem *iomem;
resource_size_t iomem_size;
 #endif
 
-   if (ncr_irq)
-   card.irq = ncr_irq;
-   if (ncr_dma)
-   card.dma = ncr_dma;
-   if (ncr_addr)
-   card.NCR5380_map_name = (NCR5380_map_type) ncr_addr;
-   if (ncr_5380)
-   card.board = BOARD_NCR5380;
-   else if (ncr_53c400)
-   card.board = BOARD_NCR53C400;
-   else if (ncr_53c400a)
-   card.board = BOARD_NCR53C400A;
-   else if (dtc_3181e)
-   card.board = BOARD_DTC3181E;
-   else if (hp_c2502)
-   card.board = BOARD_HP_C2502;
-#ifndef SCSI_G_NCR5380_MEM
-   if (isapnp_present()) {
-   struct pnp_dev *dev = NULL;
-   while ((dev = pnp_find_dev(NULL, ISAPNP_VENDOR('D', 'T', 'C'), 
ISAPNP_FUNCTION(0x436e), dev))) {
-   if (pnp_device_attach(dev) < 0)
-   continue;
-   if (pnp_activate_dev(dev) < 0) {
-   printk(KERN_ERR "dtc436e probe: activate 
failed\n");
-   pnp_device_detach(dev);
-   continue;
-   }
-   if (!pnp_port_valid(dev, 0)) {
-   printk(KERN_ERR "dtc436e probe: no valid 
port\n");
-   pnp_device_detach(dev);
-   continue;
-   }
-   if (pnp_irq_valid(dev, 0))
-   card.irq = pnp_irq(dev, 0);
-   else
-   card.irq = NO_IRQ;
-   if (pnp_dma_valid(dev, 0))
-   card.dma = pnp_dma(dev, 0);
-   else
-   card.dma = DMA_NONE;
-   card.NCR5380_map_name = (NCR5380_map_type) 
pnp_port_start(dev, 0);
-   card.board = BOARD_DTC3181E;
-   break;
-   }
-   }
-#endif
-
-   if (!(card.NCR5380_map_name))
-   return 0;
-
ports = NULL;
flags = 0;
-   switch (card.board) {
+   switch (board) {
case BOARD_NCR5380:
flags = FLAG_NO_PSEUDO_DMA | FLAG_DMA_FIXUP;
break;
@@ -191,17 +133,20 @@ static int __init generic_NCR5380_detect(struct 
scsi_host_template *tpnt)
magic_configure(-1, 0, magic);
 
region_size = 16;
-
-   if

[PATCH 3/3] g_NCR5380: Stop using scsi_module.c

2016-09-24 Thread Ondrej Zary

Convert g_NCR5380 to use scsi_add_host instead of scsi_module.c
Use pnp_driver and isa_driver to manage cards.

Signed-off-by: Ondrej Zary 
---
 drivers/scsi/g_NCR5380.c |  310 +-
 drivers/scsi/g_NCR5380.h |8 --
 2 files changed, 169 insertions(+), 149 deletions(-)

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 5162de6..6cf6b00 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -30,24 +30,25 @@
 #include "NCR5380.h"
 #include 
 #include 
-#include 
+#include 
+#include 
 #include 
 
-static int ncr_irq;
-static int ncr_dma;
-static int ncr_addr;
-static int ncr_5380;
-static int ncr_53c400;
-static int ncr_53c400a;
-static int dtc_3181e;
-static int hp_c2502;
-
-static struct card {
-   NCR5380_map_type NCR5380_map_name;
-   int irq;
-   int dma;
-   int board;  /* Use NCR53c400, Ricoh, etc. extensions ? */
-} card;
+#define MAX_CARDS 8
+
+static int irq[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+module_param_array(irq, int, NULL, 0);
+MODULE_PARM_DESC(irq, "IRQ number(s)");
+
+static int base[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+module_param_array(base, int, NULL, 0);
+MODULE_PARM_DESC(base, "base address(es)");
+
+static int card[] = { -1, -1, -1, -1, -1, -1, -1, -1 };
+module_param_array(card, int, NULL, 0);
+MODULE_PARM_DESC(card, "card type (0=NCR5380, 1=NCR53C400, 2=NCR53C400A, 
3=DTC3181E, 4=HP C2502)");
+
+MODULE_LICENSE("GPL");
 
 #ifndef SCSI_G_NCR5380_MEM
 /*
@@ -73,17 +74,9 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
 }
 #endif
 
-/**
- * generic_NCR5380_detect  -   look for NCR5380 controllers
- * @tpnt: the scsi template
- *
- * Scan for the present of NCR5380, NCR53C400, NCR53C400A, DTC3181E
- * and DTC436(ISAPnP) controllers.
- *
- * Locks: none
- */
-
-static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
+static struct Scsi_Host *generic_NCR5380_hw_init(
+   struct scsi_host_template *tpnt, struct device *pdev,
+   int base, int irq, int board)
 {
unsigned int *ports;
u8 *magic = NULL;
@@ -108,64 +101,13 @@ static int __init generic_NCR5380_detect(struct 
scsi_host_template *tpnt)
struct Scsi_Host *instance;
struct NCR5380_hostdata *hostdata;
 #ifdef SCSI_G_NCR5380_MEM
-   unsigned long base;
void __iomem *iomem;
resource_size_t iomem_size;
 #endif
 
-   if (ncr_irq)
-   card.irq = ncr_irq;
-   if (ncr_dma)
-   card.dma = ncr_dma;
-   if (ncr_addr)
-   card.NCR5380_map_name = (NCR5380_map_type) ncr_addr;
-   if (ncr_5380)
-   card.board = BOARD_NCR5380;
-   else if (ncr_53c400)
-   card.board = BOARD_NCR53C400;
-   else if (ncr_53c400a)
-   card.board = BOARD_NCR53C400A;
-   else if (dtc_3181e)
-   card.board = BOARD_DTC3181E;
-   else if (hp_c2502)
-   card.board = BOARD_HP_C2502;
-#ifndef SCSI_G_NCR5380_MEM
-   if (isapnp_present()) {
-   struct pnp_dev *dev = NULL;
-   while ((dev = pnp_find_dev(NULL, ISAPNP_VENDOR('D', 'T', 'C'), 
ISAPNP_FUNCTION(0x436e), dev))) {
-   if (pnp_device_attach(dev) < 0)
-   continue;
-   if (pnp_activate_dev(dev) < 0) {
-   printk(KERN_ERR "dtc436e probe: activate 
failed\n");
-   pnp_device_detach(dev);
-   continue;
-   }
-   if (!pnp_port_valid(dev, 0)) {
-   printk(KERN_ERR "dtc436e probe: no valid 
port\n");
-   pnp_device_detach(dev);
-   continue;
-   }
-   if (pnp_irq_valid(dev, 0))
-   card.irq = pnp_irq(dev, 0);
-   else
-   card.irq = NO_IRQ;
-   if (pnp_dma_valid(dev, 0))
-   card.dma = pnp_dma(dev, 0);
-   else
-   card.dma = DMA_NONE;
-   card.NCR5380_map_name = (NCR5380_map_type) 
pnp_port_start(dev, 0);
-   card.board = BOARD_DTC3181E;
-   break;
-   }
-   }
-#endif
-
-   if (!(card.NCR5380_map_name))
-   return 0;
-
ports = NULL;
flags = 0;
-   switch (card.board) {
+   switch (board) {
case BOARD_NCR5380:
flags = FLAG_NO_PSEUDO_DMA | FLAG_DMA_FIXUP;
break;
@@ -191,17 +133,20 @@ static int __init generic_NCR5380_detect(struct 
scsi_host_template *tpnt)
magic_configure(-1, 0, magic);
 
region_size = 16;
-
-   if (card.NCR5380_map_name != PORT_AUTO)
+

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

[PATCH 3/3] g_NCR5380: Stop using scsi_module.c

[PATCH 3/3] g_NCR5380: Stop using scsi_module.c

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

[PATCH 3/3] g_NCR5380: Stop using scsi_module.c

[PATCH 3/3] g_NCR5380: Stop using scsi_module.c

14 matches

Site Navigation

Mail list logo

Footer information