| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <200805191741.10562.bjorn.helgaas@hp.com>
Date: Mon, 19 May 2008 17:41:09 -0600
From: Bjorn Helgaas <bjorn.helgaas@...com>
To: Adam M Belay <abelay@....edu>
Cc: Len Brown <lenb@...nel.org>, linux-acpi@...r.kernel.org,
linux-kernel@...r.kernel.org, Adam Belay <ambx1@....rr.com>,
Li Shaohua <shaohua.li@...el.com>,
Matthieu Castet <castet.matthieu@...e.fr>,
Thomas Renninger <trenn@...e.de>,
Rene Herman <rene.herman@...access.nl>,
Jaroslav Kysela <perex@...ex.cz>,
Andrew Morton <akpm@...ux-foundation.org>,
Takashi Iwai <tiwai@...e.de>
Subject: Re: [patch 4/4] PNP: dont sort by type in /sys/.../resources
On Monday 19 May 2008 04:42:28 pm Adam M Belay wrote:
> Does this fix a bug or is it just a more convenient representation?
Mostly just simpler code.
It is important to maintain the order of resources within each
type, of course, so we can encode them in the correct order when
we set device configuration. But we could still sort them by
type if you think that's important.
There is some risk with changing the order, but there's also
some benefit in reducing the fiddling we do with the data from
the firmware. It's been conceptually liberating for me to see
the data correlate more exactly with _CRS and friends.
> Perhaps, if
> we're going to change the interface we could switch to PCI style sysfs
> resources.
You mean like resource_show(), which looks like this?
$ cat /sys/devices/pci0000:00/0000:00:00.0/resource
0x0000000000000000 0x0000000000000000 0x0000000000000000
0x0000000000000000 0x0000000000000000 0x0000000000000000
0x0000000000000000 0x0000000000000000 0x0000000000000000
0x0000000000000000 0x0000000000000000 0x0000000000000000
0x0000000000000000 0x0000000000000000 0x0000000000000000
0x0000000000000000 0x0000000000000000 0x0000000000000000
0x0000000000000000 0x0000000000000000 0x0000000000000000
I'm all in favor of converging with the PCI style of doing things,
but I'm not sure I understand your idea in this case. We still do
quite a bit of manual poking in the PNP resource file, and the
labels make it nicer for human consumption.
Bjorn
> Quoting Bjorn Helgaas <bjorn.helgaas@...com>:
>
> > Rather than stepping through all IO resources, then stepping through
> > all MMIO resources, etc., we can just iterate over the resource list
> > once directly.
> >
> > This can change the order in /sys, e.g.,
> >
> > # cat /sys/devices/pnp0/00:07/resources # OLD
> > state = active
> > io 0x3f8-0x3ff
> > irq 4
> >
> > # cat /sys/devices/pnp0/00:07/resources # NEW
> > state = active
> > irq 4
> > io 0x3f8-0x3ff
> >
> > The old code artificially sorted resources by type; the new code
> > just lists them in the order we read them from the ISAPNP hardware
> > or the BIOS.
> >
> > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@...com>
> >
> > Index: work10/drivers/pnp/interface.c
> > ===================================================================
> > --- work10.orig/drivers/pnp/interface.c 2008-05-05 11:54:26.000000000 -0600
> > +++ work10/drivers/pnp/interface.c 2008-05-05 11:59:53.000000000 -0600
> > @@ -248,8 +248,9 @@ static ssize_t pnp_show_current_resource
> > char *buf)
> > {
> > struct pnp_dev *dev = to_pnp_dev(dmdev);
> > + struct pnp_resource *pnp_res;
> > struct resource *res;
> > - int i, ret;
> > + int ret;
> > pnp_info_buffer_t *buffer;
> >
> > if (!dev)
> > @@ -262,46 +263,33 @@ static ssize_t pnp_show_current_resource
> > buffer->buffer = buf;
> > buffer->curr = buffer->buffer;
> >
> > - pnp_printf(buffer, "state = ");
> > - if (dev->active)
> > - pnp_printf(buffer, "active\n");
> > - else
> > - pnp_printf(buffer, "disabled\n");
> > -
> > - for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_IO, i)); i++) {
> > - pnp_printf(buffer, "io");
> > - if (res->flags & IORESOURCE_DISABLED)
> > - pnp_printf(buffer, " disabled\n");
> > - else
> > - pnp_printf(buffer, " 0x%llx-0x%llx\n",
> > - (unsigned long long) res->start,
> > - (unsigned long long) res->end);
> > - }
> > - for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_MEM, i)); i++) {
> > - pnp_printf(buffer, "mem");
> > - if (res->flags & IORESOURCE_DISABLED)
> > + pnp_printf(buffer, "state = %s\n", dev->active ? "active" : "disabled");
> > +
> > + list_for_each_entry(pnp_res, &dev->resources, list) {
> > + res = &pnp_res->res;
> > +
> > + pnp_printf(buffer, pnp_resource_type_name(res));
> > +
> > + if (res->flags & IORESOURCE_DISABLED) {
> > pnp_printf(buffer, " disabled\n");
> > - else
> > - pnp_printf(buffer, " 0x%llx-0x%llx\n",
> > + continue;
> > + }
> > +
> > + switch (pnp_resource_type(res)) {
> > + case IORESOURCE_IO:
> > + case IORESOURCE_MEM:
> > + pnp_printf(buffer, " %#llx-%#llx\n",
> > (unsigned long long) res->start,
> > (unsigned long long) res->end);
> > - }
> > - for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_IRQ, i)); i++) {
> > - pnp_printf(buffer, "irq");
> > - if (res->flags & IORESOURCE_DISABLED)
> > - pnp_printf(buffer, " disabled\n");
> > - else
> > - pnp_printf(buffer, " %lld\n",
> > - (unsigned long long) res->start);
> > - }
> > - for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_DMA, i)); i++) {
> > - pnp_printf(buffer, "dma");
> > - if (res->flags & IORESOURCE_DISABLED)
> > - pnp_printf(buffer, " disabled\n");
> > - else
> > + break;
> > + case IORESOURCE_IRQ:
> > + case IORESOURCE_DMA:
> > pnp_printf(buffer, " %lld\n",
> > (unsigned long long) res->start);
> > + break;
> > + }
> > }
> > +
> > ret = (buffer->curr - buf);
> > kfree(buffer);
> > return ret;
> >
> > --
> >
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists