linux-kernel - Re: [PATCH 1/3] hpet: convert to PNP driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 17 Mar 2010 16:00:18 -0700
From:	Bjorn Helgaas <bjorn.helgaas@...com>
To:	Venkatesh Pallipadi <venki@...gle.com>
Cc:	Clemens Ladisch <clemens@...isch.de>, linux-acpi@...r.kernel.org,
	Vojtech Pavlik <vojtech@...e.cz>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] hpet: convert to PNP driver

On Wednesday 17 March 2010 04:18:27 pm Venkatesh Pallipadi wrote:
> On Wed, Mar 17, 2010 at 10:35 AM, Bjorn Helgaas <bjorn.helgaas@...com> wrote:
> >
> > PNPACPI already parses _CRS, and does a more complete job than we did here,
> > so let's just take advantage of that.
> 
> Patch looks good.
> One comment is that we now have a dependency on CONFIG_PNP. That
> probably needs to be added in drivers/char/Kconfig. But, that also
> means we may fail for users who do not have PNP in their config today.
> May be just select PNP when HPET is selected?

Good point; I missed that.  I could just add #ifdef CONFIG_PNP around
the relevant code in hpet.c.  That's the typical way we handle this.

drivers/char/hpet.c already depends on ACPI for another reason
(acpi_irq_model), and we do select PNP whenever CONFIG_ACPI=y, so
I don't think the CONFIG_PNP=n case will arise in practice.  But
it'd probably be better to have the #ifdef wrapper than to have the
implicit dependency on the "select PNP" in drivers/acpi/Kconfig.

> > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@...com>
> > ---
> >
> >  drivers/char/hpet.c |  103 +++++++++++++--------------------------------------
> >  1 files changed, 27 insertions(+), 76 deletions(-)
> >
> >
> >
> > -       if (!data.hd_address || !data.hd_nirqs) {
> > -               printk("%s: no address or irqs in _CRS\n", __func__);
> > -               return -ENODEV;
> > -       }
> > +       data.hd_address = ioremap(mem->start, resource_size(mem));
> >
> >        return hpet_alloc(&data);
> >  }
> 
> Not a problem with this patch. But, don't we have a problem of rogue
> ioremap when hpet_alloc() fails here?

You're right.  I'll add a patch to fix that while I'm at it.

Bjorn
--
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/