linux-kernel - Re: [PATCH 1/9] PCI: Remove redundant debug output in pci_do

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]

Message-ID: <CAErSpo7+KoNNXcYBLm8C9M3RctugjzcTB6dbyeLD7R+7Jkeb1w@mail.gmail.com>
Date:	Tue, 26 Jun 2012 16:26:52 -0600
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Myron Stowe <myron.stowe@...hat.com>
Cc:	linux-pci@...r.kernel.org, linux@....linux.org.uk,
	ralf@...ux-mips.org, tglx@...utronix.de, mingo@...hat.com,
	hpa@...or.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/9] PCI: Remove redundant debug output in pci_do_fixups

On Thu, Jun 21, 2012 at 2:24 PM, Myron Stowe <myron.stowe@...hat.com> wrote:
> When the boot argument 'initcall_debug' is specified, redundant debug
> output occurs for each device as a quirk is applied:
>  ...
>  pci 0000:00:1a.0: calling quirk_usb_early_handoff+0x0/0x620
>  calling  quirk_usb_early_handoff+0x0/0x620 @ 1 for 0000:00:1a.0
>  pci fixup quirk_usb_early_handoff+0x0/0x620 returned after 32 usecs for 0000:00: 1a.0
>  ...
>
> This patch removes the redundancy by eliminating the first debug output
> occurence in the sequence shown above when 'initcall_debug' is specified.

Here's what I don't like about this: adding "initcall_debug" *removes*
some output.  My expectation is that it would only *add* output.

> Signed-off-by: Myron Stowe <myron.stowe@...hat.com>
> ---
>
>  drivers/pci/quirks.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a2d9d33..9c93558 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -2953,11 +2953,12 @@ static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f,
>                     f->vendor == (u16) PCI_ANY_ID) &&
>                    (f->device == dev->device ||
>                     f->device == (u16) PCI_ANY_ID)) {
> -                       dev_dbg(&dev->dev, "calling %pF\n", f->hook);
>                        if (initcall_debug)
>                                do_one_fixup_debug(f->hook, dev);
> -                       else
> +                       else {
> +                               dev_dbg(&dev->dev, "calling %pF\n", f->hook);
>                                f->hook(dev);

This part isn't something you changed, but I also think it's a bit
ugly that we have two possible call sites for the quirk: either inside
do_one_fixup_debug() or directly in pci_do_fixups().  I wonder if this
could be restructured a bit in the style of initcall_debug_start() and
initcall_debug_report(), so we could have this:

    ktime_t calltime;

    calltime = initcall_debug_start(dev);
    f->hook(dev);
    initcall_debug_report(dev, calltime);

where initcall_debug_report() would only print something when
initcall_debug is enabled.

> +                       }
>                }
>  }
--
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/