2008-06-01 19:38:56

by Rene Herman

[permalink] [raw]
Subject: Re: [patch 01/15] serial: when guessing, check only active resources, not options

On 31-05-08 00:48, Bjorn Helgaas wrote:

> Given a completely unknown PNP device, if it happens to have
> a modem-like string in its name and it matches a COM port
> address, we assume it's a modem.
>
> We used to check the address against all the possible resource
> options for the device. But the device is already configured
> and enabled, so we only need to check the resources it is
> actually using. If we matched an address that wasn't currently
> enabled, we would fail anyway as soon as we attempted to touch
> the device at that address.
>
> This removes a reference to the struct pnp_dev.{independent,dependent}
> fields, which I will soon make private to the PNP subsystem.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>

This isn't quite right it seems. Or changes behaviour at least:

> Index: work10/drivers/serial/8250_pnp.c
> ===================================================================
> --- work10.orig/drivers/serial/8250_pnp.c 2008-05-13 11:28:48.000000000 -0600
> +++ work10/drivers/serial/8250_pnp.c 2008-05-13 11:29:16.000000000 -0600
> @@ -383,22 +383,19 @@ static int __devinit check_name(char *na
> return 0;
> }
>
> -static int __devinit check_resources(struct pnp_option *option)
> +static int __devinit check_resources(struct pnp_dev *dev)
> {
> - struct pnp_option *tmp;
> - if (!option)
> + resource_size_t port, size;
> +
> + if (!pnp_port_valid(dev, 0))
> return 0;
>
> - for (tmp = option; tmp; tmp = tmp->next) {
> - struct pnp_port *port;
> - for (port = tmp->port; port; port = port->next)
> - if ((port->size == 8) &&
> - ((port->min == 0x2f8) ||
> - (port->min == 0x3f8) ||
> - (port->min == 0x2e8) ||
> - (port->min == 0x3e8)))
> - return 1;
> - }
> + port = pnp_port_start(dev, 0);
> + size = pnp_port_len(dev, 0);
> +
> + if (size == 8 &&
> + (port == 0x2f8 || port == 0x3f8 || port == 0x2e8 || port == 0x3e8))
> + return 1;
>
> return 0;
> }
> @@ -420,10 +417,7 @@ static int __devinit serial_pnp_guess_bo
> (dev->card && check_name(dev->card->name))))
> return -ENODEV;
>
> - if (check_resources(dev->independent))
> - return 0;
> -
> - if (check_resources(dev->dependent))
> + if (check_resources(dev))
> return 0;
>
> return -ENODEV;

We used to cry "modem" if the device _could_ be configured using a COM
address while after this change we'd only do so if it _was_ configured
using one.

My old analog modem for example had 5 dependent sets -- the first with
the regular COM port addresses (0x3f8, 0x2f8, 0x3e8, 0x2e8) and a 5th
one listing a port range of (IIRC, something like that at least) 0x100
to 0xff8.

So say I'd have a PC with the regular two onboard COM ports at COM1 and
COM2 (0x3f8 and 0x2f8) and an additional serial controller providing
COM3 and COM4 (0x3e8 and 0x2e8). My modem would then be configured to
use 0x100 and this code would no longer trigger while it did use to.

Not that I care deeply or anything but whomever wrote this did no doubt
intend it to work this way...

Rene.


2008-06-02 15:21:18

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [patch 01/15] serial: when guessing, check only active resources, not options

On Sunday 01 June 2008 01:42:01 pm Rene Herman wrote:
> We used to cry "modem" if the device _could_ be configured using a COM
> address while after this change we'd only do so if it _was_ configured
> using one.
>
> My old analog modem for example had 5 dependent sets -- the first with
> the regular COM port addresses (0x3f8, 0x2f8, 0x3e8, 0x2e8) and a 5th
> one listing a port range of (IIRC, something like that at least) 0x100
> to 0xff8.
>
> So say I'd have a PC with the regular two onboard COM ports at COM1 and
> COM2 (0x3f8 and 0x2f8) and an additional serial controller providing
> COM3 and COM4 (0x3e8 and 0x2e8). My modem would then be configured to
> use 0x100 and this code would no longer trigger while it did use to.

Ah, you're right. That scenario hadn't occurred to me. Let me see
if I can figure out a way to make that work again.

Bjorn