[PATCH] [media] winbond-cir: Fix initialization

[PATCH] [media] winbond-cir: Fix initialization

[Sean Young]

The serial driver will detect the winbond cir device as a serial port,
since it looks exactly like a serial port unless you know what it is
from the PNP ID.

Winbond CIR 00:04: Region 0x2f8-0x2ff already in use!
Winbond CIR 00:04: disabled
Winbond CIR: probe of 00:04 failed with error -16

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/winbond-cir.c | 21 ++++++++++++++++++++-
 drivers/tty/serial/8250/8250.c |  1 +
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
index 54ee348..20a0bbb 100644
--- a/drivers/media/rc/winbond-cir.c
+++ b/drivers/media/rc/winbond-cir.c
@@ -55,6 +55,7 @@
 #include <linux/slab.h>
 #include <linux/wait.h>
 #include <linux/sched.h>
+#include <linux/serial_8250.h>
 #include <media/rc-core.h>
 
 #define DRVNAME "winbond-cir"
@@ -957,6 +958,7 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id)
 	struct device *dev = &device->dev;
 	struct wbcir_data *data;
 	int err;
+	struct resource *io;
 
 	if (!(pnp_port_len(device, 0) == EHFUNC_IOMEM_LEN &&
 	      pnp_port_len(device, 1) == WAKEUP_IOMEM_LEN &&
@@ -1049,7 +1051,24 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id)
 		goto exit_release_wbase;
 	}
 
-	if (!request_region(data->sbase, SP_IOMEM_LEN, DRVNAME)) {
+	io = request_region(data->sbase, SP_IOMEM_LEN, DRVNAME);
+
+	/*
+	 * The winbond cir device looks exactly like an NS16550A serial port
+	 * unless you know what it is. We've got here via the PNP ID.
+	 */
+#ifdef CONFIG_SERIAL_8250
+	if (!io) {
+		struct uart_port port = { .iobase = data->sbase };
+		int line = serial8250_find_port(&port);
+		if (line >= 0) {
+			serial8250_unregister_port(line);
+
+			io = request_region(data->sbase, SP_IOMEM_LEN, DRVNAME);
+		}
+	}
+#endif
+	if (!io) {
 		dev_err(dev, "Region 0x%lx-0x%lx already in use!\n",
 			data->sbase, data->sbase + SP_IOMEM_LEN - 1);
 		err = -EBUSY;
diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
index 5c27f7e..d38615f 100644
--- a/drivers/tty/serial/8250/8250.c
+++ b/drivers/tty/serial/8250/8250.c
@@ -2914,6 +2914,7 @@ int serial8250_find_port(struct uart_port *p)
 	}
 	return -ENODEV;
 }
+EXPORT_SYMBOL(serial8250_find_port);
 
 #define SERIAL8250_CONSOLE	&serial8250_console
 #else
-- 
1.7.11.2

Re: [PATCH] [media] winbond-cir: Fix initialization

[Mauro Carvalho Chehab]

Hi David,

Em 31-07-2012 07:37, Sean Young escreveu:
> The serial driver will detect the winbond cir device as a serial port,
> since it looks exactly like a serial port unless you know what it is
> from the PNP ID.
> 
> Winbond CIR 00:04: Region 0x2f8-0x2ff already in use!
> Winbond CIR 00:04: disabled
> Winbond CIR: probe of 00:04 failed with error -16

Please review this patch.

> 
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/media/rc/winbond-cir.c | 21 ++++++++++++++++++++-
>  drivers/tty/serial/8250/8250.c |  1 +
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
> index 54ee348..20a0bbb 100644
> --- a/drivers/media/rc/winbond-cir.c
> +++ b/drivers/media/rc/winbond-cir.c
> @@ -55,6 +55,7 @@
>  #include <linux/slab.h>
>  #include <linux/wait.h>
>  #include <linux/sched.h>
> +#include <linux/serial_8250.h>
>  #include <media/rc-core.h>
>  
>  #define DRVNAME "winbond-cir"
> @@ -957,6 +958,7 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id)
>  	struct device *dev = &device->dev;
>  	struct wbcir_data *data;
>  	int err;
> +	struct resource *io;
>  
>  	if (!(pnp_port_len(device, 0) == EHFUNC_IOMEM_LEN &&
>  	      pnp_port_len(device, 1) == WAKEUP_IOMEM_LEN &&
> @@ -1049,7 +1051,24 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id)
>  		goto exit_release_wbase;
>  	}
>  
> -	if (!request_region(data->sbase, SP_IOMEM_LEN, DRVNAME)) {
> +	io = request_region(data->sbase, SP_IOMEM_LEN, DRVNAME);
> +
> +	/*
> +	 * The winbond cir device looks exactly like an NS16550A serial port
> +	 * unless you know what it is. We've got here via the PNP ID.
> +	 */
> +#ifdef CONFIG_SERIAL_8250
> +	if (!io) {
> +		struct uart_port port = { .iobase = data->sbase };
> +		int line = serial8250_find_port(&port);
> +		if (line >= 0) {
> +			serial8250_unregister_port(line);

Hmm... Not sure if it makes sense, but perhaps the unregistering code
should be reverting serial8250_unregister_port(line).

> +
> +			io = request_region(data->sbase, SP_IOMEM_LEN, DRVNAME);
> +		}
> +	}
> +#endif
> +	if (!io) {
>  		dev_err(dev, "Region 0x%lx-0x%lx already in use!\n",
>  			data->sbase, data->sbase + SP_IOMEM_LEN - 1);
>  		err = -EBUSY;
> diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
> index 5c27f7e..d38615f 100644
> --- a/drivers/tty/serial/8250/8250.c
> +++ b/drivers/tty/serial/8250/8250.c
> @@ -2914,6 +2914,7 @@ int serial8250_find_port(struct uart_port *p)
>  	}
>  	return -ENODEV;
>  }
> +EXPORT_SYMBOL(serial8250_find_port);
>  
>  #define SERIAL8250_CONSOLE	&serial8250_console
>  #else
> 

Re: [PATCH] [media] winbond-cir: Fix initialization

[Alan Cox]

> > +#ifdef CONFIG_SERIAL_8250

Coule be modular

> > +	if (!io) {
> > +		struct uart_port port = { .iobase = data->sbase };
> > +		int line = serial8250_find_port(&port);
> > +		if (line >= 0) {
> > +			serial8250_unregister_port(line);
> 
> Hmm... Not sure if it makes sense, but perhaps the unregistering code
> should be reverting serial8250_unregister_port(line).

Can't do that anyway it may well be busy.

> > --- a/drivers/tty/serial/8250/8250.c
> > +++ b/drivers/tty/serial/8250/8250.c
> > @@ -2914,6 +2914,7 @@ int serial8250_find_port(struct uart_port *p)
> >  	}
> >  	return -ENODEV;
> >  }
> > +EXPORT_SYMBOL(serial8250_find_port);

No - this leaks all the uart internal abstractions into the tree. We
really don't want that happening.

The right way to fix this (and a couple of other uglies) is to make 8250
on x86 scan for PnP ports *before* generic ports and to make a note of
any ports to skip on the PnP scan (so that the port poking scan ignores
them too)

Alan

Re: [PATCH] [media] winbond-cir: Fix initialization

[Greg KH]

On Tue, Jul 31, 2012 at 11:37:03AM +0100, Sean Young wrote:
> The serial driver will detect the winbond cir device as a serial port,
> since it looks exactly like a serial port unless you know what it is
> from the PNP ID.
> 
> Winbond CIR 00:04: Region 0x2f8-0x2ff already in use!
> Winbond CIR 00:04: disabled
> Winbond CIR: probe of 00:04 failed with error -16
> 
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/media/rc/winbond-cir.c | 21 ++++++++++++++++++++-
>  drivers/tty/serial/8250/8250.c |  1 +
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
> index 54ee348..20a0bbb 100644
> --- a/drivers/media/rc/winbond-cir.c
> +++ b/drivers/media/rc/winbond-cir.c
> @@ -55,6 +55,7 @@
>  #include <linux/slab.h>
>  #include <linux/wait.h>
>  #include <linux/sched.h>
> +#include <linux/serial_8250.h>
>  #include <media/rc-core.h>
>  
>  #define DRVNAME "winbond-cir"
> @@ -957,6 +958,7 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id)
>  	struct device *dev = &device->dev;
>  	struct wbcir_data *data;
>  	int err;
> +	struct resource *io;
>  
>  	if (!(pnp_port_len(device, 0) == EHFUNC_IOMEM_LEN &&
>  	      pnp_port_len(device, 1) == WAKEUP_IOMEM_LEN &&
> @@ -1049,7 +1051,24 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id)
>  		goto exit_release_wbase;
>  	}
>  
> -	if (!request_region(data->sbase, SP_IOMEM_LEN, DRVNAME)) {
> +	io = request_region(data->sbase, SP_IOMEM_LEN, DRVNAME);
> +
> +	/*
> +	 * The winbond cir device looks exactly like an NS16550A serial port
> +	 * unless you know what it is. We've got here via the PNP ID.
> +	 */
> +#ifdef CONFIG_SERIAL_8250
> +	if (!io) {
> +		struct uart_port port = { .iobase = data->sbase };
> +		int line = serial8250_find_port(&port);
> +		if (line >= 0) {
> +			serial8250_unregister_port(line);
> +
> +			io = request_region(data->sbase, SP_IOMEM_LEN, DRVNAME);
> +		}
> +	}
> +#endif
> +	if (!io) {
>  		dev_err(dev, "Region 0x%lx-0x%lx already in use!\n",
>  			data->sbase, data->sbase + SP_IOMEM_LEN - 1);
>  		err = -EBUSY;
> diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
> index 5c27f7e..d38615f 100644
> --- a/drivers/tty/serial/8250/8250.c
> +++ b/drivers/tty/serial/8250/8250.c
> @@ -2914,6 +2914,7 @@ int serial8250_find_port(struct uart_port *p)
>  	}
>  	return -ENODEV;
>  }
> +EXPORT_SYMBOL(serial8250_find_port);

EXPORT_SYMBOL_GPL please.

But can't this be done as a quirk to the 8250 driver so that it just
does not bind to this device in the first place?  Wouldn't that make
more sense?

thanks,

greg k-h

Re: [PATCH] [media] winbond-cir: Fix initialization

[Sean Young]

On Thu, Aug 16, 2012 at 09:19:09AM -0700, Greg KH wrote:
> On Tue, Jul 31, 2012 at 11:37:03AM +0100, Sean Young wrote:
> > The serial driver will detect the winbond cir device as a serial port,
> > since it looks exactly like a serial port unless you know what it is
> > from the PNP ID.
> > 
> > Winbond CIR 00:04: Region 0x2f8-0x2ff already in use!
> > Winbond CIR 00:04: disabled
> > Winbond CIR: probe of 00:04 failed with error -16
> > 
> > Signed-off-by: Sean Young <sean@mess.org>
> > ---
> >  drivers/media/rc/winbond-cir.c | 21 ++++++++++++++++++++-
> >  drivers/tty/serial/8250/8250.c |  1 +
> >  2 files changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
> > index 54ee348..20a0bbb 100644
> > --- a/drivers/media/rc/winbond-cir.c
> > +++ b/drivers/media/rc/winbond-cir.c
> > @@ -55,6 +55,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/wait.h>
> >  #include <linux/sched.h>
> > +#include <linux/serial_8250.h>
> >  #include <media/rc-core.h>
> >  
> >  #define DRVNAME "winbond-cir"
> > @@ -957,6 +958,7 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id)
> >  	struct device *dev = &device->dev;
> >  	struct wbcir_data *data;
> >  	int err;
> > +	struct resource *io;
> >  
> >  	if (!(pnp_port_len(device, 0) == EHFUNC_IOMEM_LEN &&
> >  	      pnp_port_len(device, 1) == WAKEUP_IOMEM_LEN &&
> > @@ -1049,7 +1051,24 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id)
> >  		goto exit_release_wbase;
> >  	}
> >  
> > -	if (!request_region(data->sbase, SP_IOMEM_LEN, DRVNAME)) {
> > +	io = request_region(data->sbase, SP_IOMEM_LEN, DRVNAME);
> > +
> > +	/*
> > +	 * The winbond cir device looks exactly like an NS16550A serial port
> > +	 * unless you know what it is. We've got here via the PNP ID.
> > +	 */
> > +#ifdef CONFIG_SERIAL_8250
> > +	if (!io) {
> > +		struct uart_port port = { .iobase = data->sbase };
> > +		int line = serial8250_find_port(&port);
> > +		if (line >= 0) {
> > +			serial8250_unregister_port(line);
> > +
> > +			io = request_region(data->sbase, SP_IOMEM_LEN, DRVNAME);
> > +		}
> > +	}
> > +#endif
> > +	if (!io) {
> >  		dev_err(dev, "Region 0x%lx-0x%lx already in use!\n",
> >  			data->sbase, data->sbase + SP_IOMEM_LEN - 1);
> >  		err = -EBUSY;
> > diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
> > index 5c27f7e..d38615f 100644
> > --- a/drivers/tty/serial/8250/8250.c
> > +++ b/drivers/tty/serial/8250/8250.c
> > @@ -2914,6 +2914,7 @@ int serial8250_find_port(struct uart_port *p)
> >  	}
> >  	return -ENODEV;
> >  }
> > +EXPORT_SYMBOL(serial8250_find_port);
> 
> EXPORT_SYMBOL_GPL please.

Ah yes.

> But can't this be done as a quirk to the 8250 driver so that it just
> does not bind to this device in the first place?  Wouldn't that make
> more sense?

Absolutely. However for such a quirk to work, it'll need to detect the
IR port. The problem is, based just the I/O ports for the serial port, 
it seems impossible. I've been banging my head against a brick wall 
trying to find a way to do this. As soon as you start looking at
the other I/O ports for the IR then it becomes clear, but you'll need
to glean from PNP what the port numbers are. Either that or some sort
of Super I/O detection.


Sean

Re: [PATCH] [media] winbond-cir: Fix initialization

[David Härdeman]

On Tue, Jul 31, 2012 at 11:37:03AM +0100, Sean Young wrote:
>The serial driver will detect the winbond cir device as a serial port,
>since it looks exactly like a serial port unless you know what it is
>from the PNP ID.
>
>Winbond CIR 00:04: Region 0x2f8-0x2ff already in use!
>Winbond CIR 00:04: disabled
>Winbond CIR: probe of 00:04 failed with error -16

The proposed solution means that a serial port will show up and then
automagically disappear (potentially) during boot, which isn't very
elegant.

When I discussed this a long time ago with Alan Cox (while he was still
the serial maintainer) I got the feeling that he was advocating
implementing a PNP ID based blacklist in the serial driver (apologies to
Alan if I misrepresented him now).

That seems to be a better solution (one that I never got around to
implementing myself).

>
>Signed-off-by: Sean Young <sean@mess.org>
>---
> drivers/media/rc/winbond-cir.c | 21 ++++++++++++++++++++-
> drivers/tty/serial/8250/8250.c |  1 +
> 2 files changed, 21 insertions(+), 1 deletion(-)

(BTW, I'm on vacation with sporadic Internet access for two more weeks,
and when I return I'll be spending most of my spare time moving in to a
new apartment, expect slow turnaround times for replying to emails).