linux-kernel - Re: [PATCH v2] Dynamically add and remove device specific reset functions

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] [day] [month] [year] [list]

Message-ID: <20120302092545.1719e6f2@jbarnes-desktop>
Date:	Fri, 2 Mar 2012 09:25:45 -0800
From:	Jesse Barnes <jbarnes@...tuousgeek.org>
To:	Bjorn Helgaas <bhelgaas@...gle.com>
Cc:	Tadeusz Struk <tadeusz.struk@...el.com>,
	linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
	linux-pci@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v2] Dynamically add and remove device specific reset
 functions

On Fri, 2 Mar 2012 10:17:53 -0700
Bjorn Helgaas <bhelgaas@...gle.com> wrote:

> On Fri, Mar 2, 2012 at 10:06 AM, Tadeusz Struk <tadeusz.struk@...el.com> wrote:
> > On 02/03/12 16:29, Bjorn Helgaas wrote:
> >> Where do you plan to add calls to pci_dev_specific_reset_add()?  In
> >> drivers?
> >
> > Yes, I'm working on a driver for a device with SRIOV capability.
> > I'll call it from there.
> >
> >> Did you consider adding a "reset" function pointer to struct
> >> pci_driver?  That might be more natural -- the reset function is right
> >> with all the other code that knows about the device, and there's no
> >> issue with looking up the correct reset function.
> >> With this patch, we sort of have two different ways to map
> >> vendor/device IDs to code: the usual pci_register_driver() approach,
> >> and this one using reset_list.  If pci_driver had a "reset" pointer,
> >> that would be used most of the time.  You might still need the
> >> reset_list for generic things, e.g., the reset_intel_generic_dev()
> >> function, but it would be a fallback.  It might look something like:
> >>
> >>     struct pci_driver *drv = dev->driver;
> >>
> >>     if (drv && drv->reset) {
> >>         drv->reset(dev);
> >>         return;
> >>     }
> >>
> >>     list_for_each_entry(i, &reset_list, list) {
> >>         ...
> >
> > No, I didn't think about it.
> > This is good idea, but for me the pci_dev_specific_reset() works fine.
> 
> I know your patch works fine, but I think we should have the
> discussion about whether adding a struct pci_driver pointer is a
> better long-term solution.
> 
> Greg, Jesse, others, chime in if you have any thoughts.

I thought we had one already... /me digs around.  Ah just for AER and
platform error handling.

I do like the idea of a driver hook here; I think there are quite a few
devices that can be reset w/o FLR and that may need additional
handling, so there's an opportunity to consolidate code.

I think it would probably make Tadeusz's patch smaller too; he could
just add the hook and a function for his driver, then conversions for
existing code could come later.

-- 
Jesse Barnes, Intel Open Source Technology Center

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)