| Message ID | 1245922771-1474-1-git-send-email-yu.zhao@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Yu Zhao wrote: > For devices attached to the root bus, we can't trigger Secondary Bus > Reset because there is no bridge device associated with the bus. So > need to check bus->self again NULL first before using it. > > Signed-off-by: Yu Zhao <yu.zhao@intel.com> > --- > drivers/pci/pci.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 6c93af5..20351a9 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -2171,7 +2171,7 @@ static int pci_parent_bus_reset(struct pci_dev *dev, int probe) > u16 ctrl; > struct pci_dev *pdev; > > - if (dev->subordinate) > + if (dev->subordinate || !dev->bus->self) We should use pci_is_root_bus(dev->bus) instead. Thanks, Kenji Kaneshige -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Kenji Kaneshige wrote: > Yu Zhao wrote: >> For devices attached to the root bus, we can't trigger Secondary Bus >> Reset because there is no bridge device associated with the bus. So >> need to check bus->self again NULL first before using it. >> >> Signed-off-by: Yu Zhao <yu.zhao@intel.com> >> --- >> drivers/pci/pci.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index 6c93af5..20351a9 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -2171,7 +2171,7 @@ static int pci_parent_bus_reset(struct pci_dev *dev, int probe) >> u16 ctrl; >> struct pci_dev *pdev; >> >> - if (dev->subordinate) >> + if (dev->subordinate || !dev->bus->self) > > We should use pci_is_root_bus(dev->bus) instead. Some devices (e.g. Virtual Function) might not have intermediate bridge. So I prefer check the bus->self instead of bus->parent. Thanks, Yu -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Yu Zhao wrote: > Kenji Kaneshige wrote: >> Yu Zhao wrote: >>> For devices attached to the root bus, we can't trigger Secondary Bus >>> Reset because there is no bridge device associated with the bus. So >>> need to check bus->self again NULL first before using it. >>> >>> Signed-off-by: Yu Zhao <yu.zhao@intel.com> >>> --- >>> drivers/pci/pci.c | 2 +- >>> 1 files changed, 1 insertions(+), 1 deletions(-) >>> >>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >>> index 6c93af5..20351a9 100644 >>> --- a/drivers/pci/pci.c >>> +++ b/drivers/pci/pci.c >>> @@ -2171,7 +2171,7 @@ static int pci_parent_bus_reset(struct pci_dev >>> *dev, int probe) >>> u16 ctrl; >>> struct pci_dev *pdev; >>> >>> - if (dev->subordinate) >>> + if (dev->subordinate || !dev->bus->self) >> >> We should use pci_is_root_bus(dev->bus) instead. > > Some devices (e.g. Virtual Function) might not have intermediate bridge. > So I prefer check the bus->self instead of bus->parent. > I understand the problem happens not only on the device attached to the root bus. I think the if statement need to be if (dev->subordinate || !dev->bus->self || pci_is_root_bus(bus->self)) Thanks, Kenji Kaneshige -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Kenji Kaneshige wrote: > Yu Zhao wrote: >> Kenji Kaneshige wrote: >>> Yu Zhao wrote: >>>> For devices attached to the root bus, we can't trigger Secondary Bus >>>> Reset because there is no bridge device associated with the bus. So >>>> need to check bus->self again NULL first before using it. >>>> >>>> Signed-off-by: Yu Zhao <yu.zhao@intel.com> >>>> --- >>>> drivers/pci/pci.c | 2 +- >>>> 1 files changed, 1 insertions(+), 1 deletions(-) >>>> >>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >>>> index 6c93af5..20351a9 100644 >>>> --- a/drivers/pci/pci.c >>>> +++ b/drivers/pci/pci.c >>>> @@ -2171,7 +2171,7 @@ static int pci_parent_bus_reset(struct pci_dev >>>> *dev, int probe) >>>> u16 ctrl; >>>> struct pci_dev *pdev; >>>> >>>> - if (dev->subordinate) >>>> + if (dev->subordinate || !dev->bus->self) >>> We should use pci_is_root_bus(dev->bus) instead. >> Some devices (e.g. Virtual Function) might not have intermediate bridge. >> So I prefer check the bus->self instead of bus->parent. >> > > I understand the problem happens not only on the device > attached to the root bus. I think the if statement need > to be > > if (dev->subordinate || > !dev->bus->self || pci_is_root_bus(bus->self)) Yes, this works too. Just wonder if there is a specific reason for this. (i.e. why can't !dev->bus->self cover pci_is_root_bus(dev->bus) case?) Thanks, Yu -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Yu Zhao wrote: > Kenji Kaneshige wrote: >> Yu Zhao wrote: >>> Kenji Kaneshige wrote: >>>> Yu Zhao wrote: >>>>> For devices attached to the root bus, we can't trigger Secondary Bus >>>>> Reset because there is no bridge device associated with the bus. So >>>>> need to check bus->self again NULL first before using it. >>>>> >>>>> Signed-off-by: Yu Zhao <yu.zhao@intel.com> >>>>> --- >>>>> drivers/pci/pci.c | 2 +- >>>>> 1 files changed, 1 insertions(+), 1 deletions(-) >>>>> >>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >>>>> index 6c93af5..20351a9 100644 >>>>> --- a/drivers/pci/pci.c >>>>> +++ b/drivers/pci/pci.c >>>>> @@ -2171,7 +2171,7 @@ static int pci_parent_bus_reset(struct >>>>> pci_dev *dev, int probe) >>>>> u16 ctrl; >>>>> struct pci_dev *pdev; >>>>> >>>>> - if (dev->subordinate) >>>>> + if (dev->subordinate || !dev->bus->self) >>>> We should use pci_is_root_bus(dev->bus) instead. >>> Some devices (e.g. Virtual Function) might not have intermediate >>> bridge. So I prefer check the bus->self instead of bus->parent. >>> >> >> I understand the problem happens not only on the device >> attached to the root bus. I think the if statement need >> to be >> >> if (dev->subordinate || >> !dev->bus->self || pci_is_root_bus(bus->self)) > > Yes, this works too. Just wonder if there is a specific reason for this. > (i.e. why can't !dev->bus->self cover pci_is_root_bus(dev->bus) case?) > There are some platforms whose root bus's bus->self points the device corresponding to host bridge. So we cannot check bus->self to see if the bus is root or not. This is the reason why the pci_is_root_bus() is introduced recently. Actually, I have a experience that I made a trouble related to this (I checked bus->self to see if the bus is root). Thanks, Kenji Kaneshige -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Kenji Kaneshige wrote: > Yu Zhao wrote: >> Kenji Kaneshige wrote: >>> Yu Zhao wrote: >>>> Kenji Kaneshige wrote: >>>>> Yu Zhao wrote: >>>>>> For devices attached to the root bus, we can't trigger Secondary Bus >>>>>> Reset because there is no bridge device associated with the bus. So >>>>>> need to check bus->self again NULL first before using it. >>>>>> >>>>>> Signed-off-by: Yu Zhao <yu.zhao@intel.com> >>>>>> --- >>>>>> drivers/pci/pci.c | 2 +- >>>>>> 1 files changed, 1 insertions(+), 1 deletions(-) >>>>>> >>>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >>>>>> index 6c93af5..20351a9 100644 >>>>>> --- a/drivers/pci/pci.c >>>>>> +++ b/drivers/pci/pci.c >>>>>> @@ -2171,7 +2171,7 @@ static int pci_parent_bus_reset(struct >>>>>> pci_dev *dev, int probe) >>>>>> u16 ctrl; >>>>>> struct pci_dev *pdev; >>>>>> >>>>>> - if (dev->subordinate) >>>>>> + if (dev->subordinate || !dev->bus->self) >>>>> We should use pci_is_root_bus(dev->bus) instead. >>>> Some devices (e.g. Virtual Function) might not have intermediate >>>> bridge. So I prefer check the bus->self instead of bus->parent. >>>> >>> I understand the problem happens not only on the device >>> attached to the root bus. I think the if statement need >>> to be >>> >>> if (dev->subordinate || >>> !dev->bus->self || pci_is_root_bus(bus->self)) >> Yes, this works too. Just wonder if there is a specific reason for this. >> (i.e. why can't !dev->bus->self cover pci_is_root_bus(dev->bus) case?) >> > > There are some platforms whose root bus's bus->self points > the device corresponding to host bridge. So we cannot check > bus->self to see if the bus is root or not. This is the > reason why the pci_is_root_bus() is introduced recently. > > Actually, I have a experience that I made a trouble related > to this (I checked bus->self to see if the bus is root). Ok, it's clear now. Thank you for the explanation! -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 26, 2009 at 09:10:18AM +0800, Yu Zhao wrote: >>> @@ -2171,7 +2171,7 @@ static int pci_parent_bus_reset(struct pci_dev *dev, int probe) >>> u16 ctrl; >>> struct pci_dev *pdev; >>> - if (dev->subordinate) >>> + if (dev->subordinate || !dev->bus->self) >> >> We should use pci_is_root_bus(dev->bus) instead. > > Some devices (e.g. Virtual Function) might not have intermediate bridge. > So I prefer check the bus->self instead of bus->parent. That doesn't sound right to me. Virtual Functions should have the same parent as the physical function which provides them. Even if they're on a different bus number.
Matthew Wilcox wrote: > On Fri, Jun 26, 2009 at 09:10:18AM +0800, Yu Zhao wrote: >>>> @@ -2171,7 +2171,7 @@ static int pci_parent_bus_reset(struct pci_dev *dev, int probe) >>>> u16 ctrl; >>>> struct pci_dev *pdev; >>>> - if (dev->subordinate) >>>> + if (dev->subordinate || !dev->bus->self) >>> We should use pci_is_root_bus(dev->bus) instead. >> Some devices (e.g. Virtual Function) might not have intermediate bridge. >> So I prefer check the bus->self instead of bus->parent. > > That doesn't sound right to me. Virtual Functions should have the same > parent as the physical function which provides them. Even if they're > on a different bus number. Yes, VFs do have same bus parent as the PF they are associated with. However, the bus that VFs reside on may not have the bridge device (bus->self). Thanks, Yu -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 6c93af5..20351a9 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -2171,7 +2171,7 @@ static int pci_parent_bus_reset(struct pci_dev *dev, int probe) u16 ctrl; struct pci_dev *pdev; - if (dev->subordinate) + if (dev->subordinate || !dev->bus->self) return -ENOTTY; list_for_each_entry(pdev, &dev->bus->devices, bus_list)
For devices attached to the root bus, we can't trigger Secondary Bus Reset because there is no bridge device associated with the bus. So need to check bus->self again NULL first before using it. Signed-off-by: Yu Zhao <yu.zhao@intel.com> --- drivers/pci/pci.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)