* [PATCH v2] Dynamically add and remove device specific reset functions @ 2012-03-02 11:55 tadeusz.struk 2012-03-02 16:29 ` Bjorn Helgaas 0 siblings, 1 reply; 5+ messages in thread From: tadeusz.struk @ 2012-03-02 11:55 UTC (permalink / raw) To: jbarnes, bhelgaas; +Cc: linux-kernel, kvm, linux-pci, tadeusz.struk Hi, Reworked according to comments. I have a use case where I need to cleanup resource allocated for Virtual Functions after a guest OS that used it crashed. This cleanup needs to be done before the VF is being FLRed. The only possible way to do this seems to be by using pci_dev_specific_reset() function. Unfortunately this function only works for devices defined in a static table in the drivers/pci/quirks.c file. This patch changes it so that specific reset functions can be added and deleted dynamically. Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com> --- drivers/pci/pci.h | 14 +++++++++++ drivers/pci/quirks.c | 61 +++++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 65 insertions(+), 10 deletions(-) diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 1009a5e..3e95df6 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -312,6 +312,7 @@ static inline resource_size_t pci_resource_alignment(struct pci_dev *dev, extern void pci_enable_acs(struct pci_dev *dev); struct pci_dev_reset_methods { + struct list_head list; u16 vendor; u16 device; int (*reset)(struct pci_dev *dev, int probe); @@ -319,11 +320,24 @@ struct pci_dev_reset_methods { #ifdef CONFIG_PCI_QUIRKS extern int pci_dev_specific_reset(struct pci_dev *dev, int probe); +extern int pci_dev_specific_reset_add(struct pci_dev_reset_methods + *reset_method); +extern int pci_dev_specific_reset_remove(struct pci_dev_reset_methods + *reset_method); #else static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe) { return -ENOTTY; } +int pci_dev_specific_reset_add(struct pci_dev_reset_methods *reset_method) +{ + return 0; +} +int +pci_dev_specific_reset_remove(struct pci_dev_reset_methods *reset_method) +{ + return 0; +} #endif #endif /* DRIVERS_PCI_H */ diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 6476547..a0152fd 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -3070,26 +3070,67 @@ static int reset_intel_82599_sfp_virtfn(struct pci_dev *dev, int probe) } #define PCI_DEVICE_ID_INTEL_82599_SFP_VF 0x10ed - -static const struct pci_dev_reset_methods pci_dev_reset_methods[] = { - { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82599_SFP_VF, - reset_intel_82599_sfp_virtfn }, - { PCI_VENDOR_ID_INTEL, PCI_ANY_ID, +static struct pci_dev_reset_methods pci_dev_reset_methods[] = { + { {NULL, NULL}, PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82599_SFP_VF, + reset_intel_82599_sfp_virtfn }, + { {NULL, NULL}, PCI_VENDOR_ID_INTEL, PCI_ANY_ID, reset_intel_generic_dev }, - { 0 } }; +static DEFINE_SPINLOCK(reset_list_lock); +static LIST_HEAD(reset_list); + +static int __init pci_dev_specific_reset_init(void) +{ + int i; + for (i = 0; i < ARRAY_SIZE(pci_dev_reset_methods); i++) { + pci_dev_specific_reset_add(&pci_dev_reset_methods[i]); + } + return 0; +} + +late_initcall(pci_dev_specific_reset_init); + +int pci_dev_specific_reset_add(struct pci_dev_reset_methods *reset_method) +{ + WARN_ON(!reset_method->reset); + INIT_LIST_HEAD(&reset_method->list); + spin_lock(&reset_list_lock); + list_add(&reset_method->list, &reset_list); + spin_unlock(&reset_list_lock); + return 0; +} +EXPORT_SYMBOL(pci_dev_specific_reset_add); + +int pci_dev_specific_reset_remove(struct pci_dev_reset_methods *reset_method) +{ + spin_lock(&reset_list_lock); + list_del(&reset_method->list); + spin_unlock(&reset_list_lock); + return 0; +} +EXPORT_SYMBOL(pci_dev_specific_reset_remove); + int pci_dev_specific_reset(struct pci_dev *dev, int probe) { const struct pci_dev_reset_methods *i; + spin_lock(&reset_list_lock); + list_for_each_entry(i, &reset_list, list) { + if (i->vendor == dev->vendor && + i->device == dev->device) { + spin_unlock(&reset_list_lock); + return i->reset(dev, probe); + } + } - for (i = pci_dev_reset_methods; i->reset; i++) { + list_for_each_entry(i, &reset_list, list) { if ((i->vendor == dev->vendor || i->vendor == (u16)PCI_ANY_ID) && - (i->device == dev->device || - i->device == (u16)PCI_ANY_ID)) + i->device == (u16)PCI_ANY_ID) { + spin_unlock(&reset_list_lock); return i->reset(dev, probe); + } } - + spin_unlock(&reset_list_lock); return -ENOTTY; } -- 1.7.7.6 -------------------------------------------------------------- Intel Shannon Limited Registered in Ireland Registered Office: Collinstown Industrial Park, Leixlip, County Kildare Registered Number: 308263 Business address: Dromore House, East Park, Shannon, Co. Clare This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Dynamically add and remove device specific reset functions 2012-03-02 11:55 [PATCH v2] Dynamically add and remove device specific reset functions tadeusz.struk @ 2012-03-02 16:29 ` Bjorn Helgaas 2012-03-02 17:06 ` Tadeusz Struk 0 siblings, 1 reply; 5+ messages in thread From: Bjorn Helgaas @ 2012-03-02 16:29 UTC (permalink / raw) To: tadeusz.struk; +Cc: jbarnes, linux-kernel, kvm, linux-pci Thanks! You didn't fix the subject line :) It should contain "PCI: ". Thanks for including the "v2" inside "[PATCH]". That helps keep things straight. On Fri, Mar 2, 2012 at 4:55 AM, <tadeusz.struk@intel.com> wrote: > Hi, > Reworked according to comments. All the text here becomes part of the changelog, so you don't need to include the stuff that's only relevant during review, e.g., "Hi, Reworked..." That sort of stuff can either go in a separate "cover" email ([0/1], with the patch itself being [1/1]), or after the "---" along with the diffstat (text after the "---" doesn't go in the changelog). > I have a use case where I need to cleanup resource allocated for Virtual > Functions after a guest OS that used it crashed. This cleanup needs to > be done before the VF is being FLRed. The only possible way to do this > seems to be by using pci_dev_specific_reset() function. Unfortunately > this function only works for devices defined in a static table in the > drivers/pci/quirks.c file. This patch changes it so that specific reset > functions can be added and deleted dynamically. > > Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com> > > --- > drivers/pci/pci.h | 14 +++++++++++ > drivers/pci/quirks.c | 61 +++++++++++++++++++++++++++++++++++++++++-------- > 2 files changed, 65 insertions(+), 10 deletions(-) > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 1009a5e..3e95df6 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -312,6 +312,7 @@ static inline resource_size_t pci_resource_alignment(struct pci_dev *dev, > extern void pci_enable_acs(struct pci_dev *dev); > > struct pci_dev_reset_methods { > + struct list_head list; > u16 vendor; > u16 device; > int (*reset)(struct pci_dev *dev, int probe); > @@ -319,11 +320,24 @@ struct pci_dev_reset_methods { > > #ifdef CONFIG_PCI_QUIRKS > extern int pci_dev_specific_reset(struct pci_dev *dev, int probe); > +extern int pci_dev_specific_reset_add(struct pci_dev_reset_methods > + *reset_method); > +extern int pci_dev_specific_reset_remove(struct pci_dev_reset_methods > + *reset_method); > #else > static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe) > { > return -ENOTTY; > } > +int pci_dev_specific_reset_add(struct pci_dev_reset_methods *reset_method) > +{ > + return 0; > +} > +int > +pci_dev_specific_reset_remove(struct pci_dev_reset_methods *reset_method) > +{ > + return 0; > +} > #endif > > #endif /* DRIVERS_PCI_H */ > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index 6476547..a0152fd 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -3070,26 +3070,67 @@ static int reset_intel_82599_sfp_virtfn(struct pci_dev *dev, int probe) > } > > #define PCI_DEVICE_ID_INTEL_82599_SFP_VF 0x10ed > - > -static const struct pci_dev_reset_methods pci_dev_reset_methods[] = { > - { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82599_SFP_VF, > - reset_intel_82599_sfp_virtfn }, > - { PCI_VENDOR_ID_INTEL, PCI_ANY_ID, > +static struct pci_dev_reset_methods pci_dev_reset_methods[] = { > + { {NULL, NULL}, PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82599_SFP_VF, I don't think you need the "{NULL, NULL}" initializers here. In fact, it encodes knowledge of the struct list_head implementation that should not be here. If you needed to intialize a list_head in a structure like this, you would use "LIST_HEAD_INIT," but I don't think you need it here. I guess you probably would have to add "list" to the end, not the beginning, of struct pci_dev_reset_methods for this to work. But putting it at the end should be fine. > + reset_intel_82599_sfp_virtfn }, > + { {NULL, NULL}, PCI_VENDOR_ID_INTEL, PCI_ANY_ID, > reset_intel_generic_dev }, > - { 0 } > }; > > +static DEFINE_SPINLOCK(reset_list_lock); > +static LIST_HEAD(reset_list); > + > +static int __init pci_dev_specific_reset_init(void) > +{ > + int i; Add a blank line here. > + for (i = 0; i < ARRAY_SIZE(pci_dev_reset_methods); i++) { Linux style omits the curly brackets around simple single-line blocks like this: > + pci_dev_specific_reset_add(&pci_dev_reset_methods[i]); > + } > + return 0; > +} > + > +late_initcall(pci_dev_specific_reset_init); > + > +int pci_dev_specific_reset_add(struct pci_dev_reset_methods *reset_method) > +{ > + WARN_ON(!reset_method->reset); > + INIT_LIST_HEAD(&reset_method->list); > + spin_lock(&reset_list_lock); > + list_add(&reset_method->list, &reset_list); > + spin_unlock(&reset_list_lock); > + return 0; > +} > +EXPORT_SYMBOL(pci_dev_specific_reset_add); > + > +int pci_dev_specific_reset_remove(struct pci_dev_reset_methods *reset_method) > +{ > + spin_lock(&reset_list_lock); > + list_del(&reset_method->list); > + spin_unlock(&reset_list_lock); > + return 0; > +} > +EXPORT_SYMBOL(pci_dev_specific_reset_remove); > + > int pci_dev_specific_reset(struct pci_dev *dev, int probe) > { > const struct pci_dev_reset_methods *i; > + spin_lock(&reset_list_lock); > + list_for_each_entry(i, &reset_list, list) { > + if (i->vendor == dev->vendor && > + i->device == dev->device) { > + spin_unlock(&reset_list_lock); > + return i->reset(dev, probe); Where do you plan to add calls to pci_dev_specific_reset_add()? In drivers? 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) { ... In that case, you might not even need the ability to dynamically add things to the list, since the only things to add would be "generic" things that would be more static. Perhaps this approach was previously discussed and discarded; if so, I missed it. Bjorn > + } > + } > > - for (i = pci_dev_reset_methods; i->reset; i++) { > + list_for_each_entry(i, &reset_list, list) { > if ((i->vendor == dev->vendor || > i->vendor == (u16)PCI_ANY_ID) && > - (i->device == dev->device || > - i->device == (u16)PCI_ANY_ID)) > + i->device == (u16)PCI_ANY_ID) { > + spin_unlock(&reset_list_lock); > return i->reset(dev, probe); > + } > } > - > + spin_unlock(&reset_list_lock); > return -ENOTTY; > } > -- > 1.7.7.6 > > -------------------------------------------------------------- > Intel Shannon Limited > Registered in Ireland > Registered Office: Collinstown Industrial Park, Leixlip, County Kildare > Registered Number: 308263 > Business address: Dromore House, East Park, Shannon, Co. Clare > > This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Dynamically add and remove device specific reset functions 2012-03-02 16:29 ` Bjorn Helgaas @ 2012-03-02 17:06 ` Tadeusz Struk 2012-03-02 17:17 ` Bjorn Helgaas 0 siblings, 1 reply; 5+ messages in thread From: Tadeusz Struk @ 2012-03-02 17:06 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: jbarnes, linux-kernel, kvm, linux-pci, Struk, Tadeusz 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. > In that case, you might not even need the ability to dynamically add > things to the list, since the only things to add would be "generic" > things that would be more static. > > Perhaps this approach was previously discussed and discarded; if so, I > missed it. > > Bjorn Do you want me to send another patch that fixes these minor issues you pointed out? Thanks for you comments. Tadeusz -------------------------------------------------------------- Intel Shannon Limited Registered in Ireland Registered Office: Collinstown Industrial Park, Leixlip, County Kildare Registered Number: 308263 Business address: Dromore House, East Park, Shannon, Co. Clare This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Dynamically add and remove device specific reset functions 2012-03-02 17:06 ` Tadeusz Struk @ 2012-03-02 17:17 ` Bjorn Helgaas 2012-03-02 17:25 ` Jesse Barnes 0 siblings, 1 reply; 5+ messages in thread From: Bjorn Helgaas @ 2012-03-02 17:17 UTC (permalink / raw) To: Tadeusz Struk; +Cc: jbarnes, linux-kernel, kvm, linux-pci, Greg Kroah-Hartman On Fri, Mar 2, 2012 at 10:06 AM, Tadeusz Struk <tadeusz.struk@intel.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. Bjorn ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Dynamically add and remove device specific reset functions 2012-03-02 17:17 ` Bjorn Helgaas @ 2012-03-02 17:25 ` Jesse Barnes 0 siblings, 0 replies; 5+ messages in thread From: Jesse Barnes @ 2012-03-02 17:25 UTC (permalink / raw) To: Bjorn Helgaas Cc: Tadeusz Struk, linux-kernel, kvm, linux-pci, Greg Kroah-Hartman [-- Attachment #1: Type: text/plain, Size: 2251 bytes --] On Fri, 2 Mar 2012 10:17:53 -0700 Bjorn Helgaas <bhelgaas@google.com> wrote: > On Fri, Mar 2, 2012 at 10:06 AM, Tadeusz Struk <tadeusz.struk@intel.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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-03-02 17:25 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-03-02 11:55 [PATCH v2] Dynamically add and remove device specific reset functions tadeusz.struk 2012-03-02 16:29 ` Bjorn Helgaas 2012-03-02 17:06 ` Tadeusz Struk 2012-03-02 17:17 ` Bjorn Helgaas 2012-03-02 17:25 ` Jesse Barnes
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).