| 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: <CAErSpo6VxdvpmSA_RbPLcqduwqXbJg_VCqSdW7_+_UGLeWWT+w@mail.gmail.com>
Date: Tue, 27 Mar 2012 14:32:31 -0600
From: Bjorn Helgaas <bhelgaas@...gle.com>
To: Witold Szczeponik <Witold.Szczeponik@....net>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] PNP: Simplify setting of resources
On Tue, Mar 20, 2012 at 1:57 PM, Witold Szczeponik
<Witold.Szczeponik@....net> wrote:
> This patch factors out the setting of PNP resources into one function which is
> then reused for all PNP resource types. This makes the code more concise and
> avoids duplication. The parameters "type" and "flags" are not used at the
> moment but will be used by follow-up patches. Placeholders for these patches
> can be found in the comment lines that contain the "TBD" marker.
>
> As the code does not make any changes to the ABI, no regressions are expected.
>
> NB: While at it, support for bus type resources is added.
>
> The patch is applied against Linux 3.3.x.
A few minor comments below, but I think this looks good. In the past,
I've sent PNP patches through linux-acpi and Len's ACPI tree.
Reviewed-by: Bjorn Helgaas <bhelgaas@...gle.com>
> Signed-off-by: Witold Szczeponik <Witold.Szczeponik@....net>
>
>
> Index: linux/drivers/pnp/interface.c
> ===================================================================
> --- linux.orig/drivers/pnp/interface.c
> +++ linux/drivers/pnp/interface.c
> @@ -298,6 +298,39 @@ static ssize_t pnp_show_current_resource
> return ret;
> }
>
> +static char *pnp_get_resource_value(char *buf,
> + unsigned long type,
> + resource_size_t *start,
> + resource_size_t *end,
> + unsigned long *flags)
> +{
> + if (start)
> + *start = 0;
> + if (end)
> + *end = 0;
> + if (flags)
> + *flags = 0;
> +
> + /* TBD: allow for disabled resources */
> +
> + buf = skip_spaces(buf);
> + if (start) {
> + *start = simple_strtoull(buf, &buf, 0);
> + if (end) {
> + buf = skip_spaces(buf);
> + if (*buf == '-') {
> + buf = skip_spaces(buf + 1);
> + *end = simple_strtoull(buf, &buf, 0);
> + } else
> + *end = *start;
> + }
> + }
> +
> + /* TBD: allow for additional flags, e.g., IORESOURCE_WINDOW */
> +
> + return buf;
> +}
> +
> static ssize_t pnp_set_current_resources(struct device *dmdev,
> struct device_attribute *attr,
> const char *ubuf, size_t count)
> @@ -305,7 +338,6 @@ static ssize_t pnp_set_current_resources
> struct pnp_dev *dev = to_pnp_dev(dmdev);
> char *buf = (void *)ubuf;
> int retval = 0;
> - resource_size_t start, end;
>
> if (dev->status & PNP_ATTACHED) {
> retval = -EBUSY;
> @@ -349,6 +381,10 @@ static ssize_t pnp_set_current_resources
> goto done;
> }
> if (!strnicmp(buf, "set", 3)) {
> + resource_size_t start;
> + resource_size_t end;
> + unsigned long flags;
> +
> if (dev->active)
> goto done;
> buf += 3;
> @@ -357,42 +393,42 @@ static ssize_t pnp_set_current_resources
> while (1) {
> buf = skip_spaces(buf);
> if (!strnicmp(buf, "io", 2)) {
> - buf = skip_spaces(buf + 2);
> - start = simple_strtoul(buf, &buf, 0);
> - buf = skip_spaces(buf);
> - if (*buf == '-') {
> - buf = skip_spaces(buf + 1);
> - end = simple_strtoul(buf, &buf, 0);
> - } else
> - end = start;
> - pnp_add_io_resource(dev, start, end, 0);
> - continue;
> - }
> - if (!strnicmp(buf, "mem", 3)) {
> - buf = skip_spaces(buf + 3);
> - start = simple_strtoul(buf, &buf, 0);
> - buf = skip_spaces(buf);
> - if (*buf == '-') {
> - buf = skip_spaces(buf + 1);
> - end = simple_strtoul(buf, &buf, 0);
> - } else
> - end = start;
> - pnp_add_mem_resource(dev, start, end, 0);
> - continue;
> - }
> - if (!strnicmp(buf, "irq", 3)) {
> - buf = skip_spaces(buf + 3);
> - start = simple_strtoul(buf, &buf, 0);
> - pnp_add_irq_resource(dev, start, 0);
> - continue;
> - }
> - if (!strnicmp(buf, "dma", 3)) {
> - buf = skip_spaces(buf + 3);
> - start = simple_strtoul(buf, &buf, 0);
> - pnp_add_dma_resource(dev, start, 0);
> - continue;
> - }
> - break;
> + buf = pnp_get_resource_value(buf + 2,
> + IORESOURCE_IO,
> + &start,
> + &end,
> + &flags);
Looks like at least "&end" will fit on the same line as "&start".
> + pnp_add_io_resource(dev, start, end, flags);
> + } else if (!strnicmp(buf, "mem", 3)) {
> + buf = pnp_get_resource_value(buf + 3,
> + IORESOURCE_MEM,
> + &start,
> + &end,
> + &flags);
> + pnp_add_mem_resource(dev, start, end, flags);
> + } else if (!strnicmp(buf, "irq", 3)) {
> + buf = pnp_get_resource_value(buf + 3,
> + IORESOURCE_IRQ,
> + &start,
> + NULL,
> + &flags);
> + pnp_add_irq_resource(dev, start, flags);
> + } else if (!strnicmp(buf, "dma", 3)) {
> + buf = pnp_get_resource_value(buf + 3,
> + IORESOURCE_DMA,
> + &start,
> + NULL,
> + &flags);
> + pnp_add_dma_resource(dev, start, flags);
> + } else if (!strnicmp(buf, "bus", 3)) {
> + buf = pnp_get_resource_value(buf + 3,
> + IORESOURCE_BUS,
> + &start,
> + &end,
> + NULL);
> + pnp_add_bus_resource(dev, start, end);
It makes sense to allow a user to set bus number resources just like
any other resources, but I don't think the rest of PNP supports this
yet. I think only PNPACPI supports bus number resources at all, so
when you "activate" the device after setting its resources, we'll call
pnpacpi_set_resources() and pnpacpi_encode_resources(), but
pnpacpi_encode_resources() doesn't know how to encode the address16
descriptor that will probably be used for bus numbers.
I guess that's all right, though. We already have the same situation
for all the address space descriptors when used for IO or MEM.
> + } else
> + break;
> }
> mutex_unlock(&pnp_res_mutex);
> goto done;
Powered by blists - more mailing lists