Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
On 12/18/2017 6:26 AM, Christoph Hellwig wrote: On Fri, Dec 15, 2017 at 12:18:02PM -0600, Bjorn Helgaas wrote: I think Christoph volunteered to do some restructuring, but I don't know his timeframe. If you can, I would probably wait for that because there's so much overlap here. I'll have some time over the holidays. If you need it more urgent than that feel free to take over. We will wait for your changes. Thanks Christoph. Cheers GOVINDA
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
On 12/18/2017 6:26 AM, Christoph Hellwig wrote: On Fri, Dec 15, 2017 at 12:18:02PM -0600, Bjorn Helgaas wrote: I think Christoph volunteered to do some restructuring, but I don't know his timeframe. If you can, I would probably wait for that because there's so much overlap here. I'll have some time over the holidays. If you need it more urgent than that feel free to take over. We will wait for your changes. Thanks Christoph. Cheers GOVINDA
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
On Fri, Dec 15, 2017 at 12:18:02PM -0600, Bjorn Helgaas wrote: > I think Christoph volunteered to do some restructuring, but I don't > know his timeframe. If you can, I would probably wait for that > because there's so much overlap here. I'll have some time over the holidays. If you need it more urgent than that feel free to take over.
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
On Fri, Dec 15, 2017 at 12:18:02PM -0600, Bjorn Helgaas wrote: > I think Christoph volunteered to do some restructuring, but I don't > know his timeframe. If you can, I would probably wait for that > because there's so much overlap here. I'll have some time over the holidays. If you need it more urgent than that feel free to take over.
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
On 16/12/17 05:18, Bjorn Helgaas wrote:
> [+cc Russell, Sinan, Herbert, Srikanth, Derek, Satanand, Felix, Raghu]
>
> On Fri, Dec 15, 2017 at 09:48:02AM -0600, Govinda Tatti wrote:
>> On 12/13/2017 3:24 PM, Bjorn Helgaas wrote:
>>> On Wed, Dec 13, 2017 at 02:46:57PM -0600, Govinda Tatti wrote:
>
-static bool pcie_has_flr(struct pci_dev *dev)
+bool pcie_has_flr(struct pci_dev *dev)
{
u32 cap;
@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev)
pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
return cap & PCI_EXP_DEVCAP_FLR;
}
+EXPORT_SYMBOL_GPL(pcie_has_flr);
>
>>> I'd rather change pcie_flr() so you could *always* call it, and
>>> it would return 0, -ENOTTY, or whatever, based on whether FLR
>>> is supported. Is that feasible?
>
>> Sure, I will add pcie_has_flr() logic inside pcie_flr() and
>> return appropriate values as suggested by you. Do we still want
>> to retain pcie_has_flr() and its usage inside pci.c?.Otherwise,
>> I will remove it and do required cleanup.
>
> If you can restructure the code and remove pcie_has_flr() while
> retaining the existing behavior of its callers, that would be
> great.
>
I checked the current usage of pcie_has_flr() and pcie_flr(). I
have a couple of questions or need some clarification.
1. pcie_has_flr() usage inside pci_probe_reset_function().
This function is only calling pcie_has_flr() but not pcie_flr().
Rest of the code is trying to do specific type of reset except
pcie_flr().
rc = pci_dev_specific_reset(dev, 1);
if (rc != -ENOTTY)
return rc;
if (pcie_has_flr(dev))
return 0;
rc = pci_af_flr(dev, 1);
if (rc != -ENOTTY)
return rc;
In other-words, I can remove usage of pcie_has_flr() in all
other places in pci.c except in above function.
>
>>> I think we should keep the EXPORT_SYMBOL_GPL() part of a60a2b73ba69
>>> ("PCI: Export pcie_flr()"), but revert the restructuring part.
>>>
>>> Prior to a60a2b73ba69, we had
>>>
>>> int pcie_flr(struct pci_dev *dev, int probe);
>>>
>>> like all the other reset methods. AFAICT, the addition of
>>> pcie_has_flr() was to optimize the path slightly because when
>>> drivers call pcie_flr(), they should already know that their
>>> hardware supports FLR. But I don't think that optimization is
>>> worth the extra code complexity. If we do need to optimize it, we
>>> can check this in the core during enumeration and set
>>> PCI_DEV_FLAGS_NO_FLR_RESET accordingly.
>
>> Not all code paths are aware of FLR capability and also, not
>> using pcie_flr(). For example,
>>
>> arch/powerpc/platforms/powernv/eeh-powernv.c
>
> I assume you're referring to pnv_eeh_do_flr() (which contains code similar
> to pcie_flr()) and pnv_eeh_do_af_flr() (which has code similar to
> pci_af_flr()). I agree that those are problematic and would ideally be
> unified with the PCI core implementations.
>
> Powerpc has quite a bit of this sort of special-case code for several
> reasons, some just historical and some more concrete, so I don't know how
> feasible this is.
It would be lovely if pnv-eeh code used pci_af_flr() but since
pnv_eeh_do_flr() uses different config space accessors (not sure why
exactly, probably to avoid freezing the entire PHB), it is harder than just
trivial change. I'll try and have a deeper look though.
--
Alexey
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
On 16/12/17 05:18, Bjorn Helgaas wrote:
> [+cc Russell, Sinan, Herbert, Srikanth, Derek, Satanand, Felix, Raghu]
>
> On Fri, Dec 15, 2017 at 09:48:02AM -0600, Govinda Tatti wrote:
>> On 12/13/2017 3:24 PM, Bjorn Helgaas wrote:
>>> On Wed, Dec 13, 2017 at 02:46:57PM -0600, Govinda Tatti wrote:
>
-static bool pcie_has_flr(struct pci_dev *dev)
+bool pcie_has_flr(struct pci_dev *dev)
{
u32 cap;
@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev)
pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
return cap & PCI_EXP_DEVCAP_FLR;
}
+EXPORT_SYMBOL_GPL(pcie_has_flr);
>
>>> I'd rather change pcie_flr() so you could *always* call it, and
>>> it would return 0, -ENOTTY, or whatever, based on whether FLR
>>> is supported. Is that feasible?
>
>> Sure, I will add pcie_has_flr() logic inside pcie_flr() and
>> return appropriate values as suggested by you. Do we still want
>> to retain pcie_has_flr() and its usage inside pci.c?.Otherwise,
>> I will remove it and do required cleanup.
>
> If you can restructure the code and remove pcie_has_flr() while
> retaining the existing behavior of its callers, that would be
> great.
>
I checked the current usage of pcie_has_flr() and pcie_flr(). I
have a couple of questions or need some clarification.
1. pcie_has_flr() usage inside pci_probe_reset_function().
This function is only calling pcie_has_flr() but not pcie_flr().
Rest of the code is trying to do specific type of reset except
pcie_flr().
rc = pci_dev_specific_reset(dev, 1);
if (rc != -ENOTTY)
return rc;
if (pcie_has_flr(dev))
return 0;
rc = pci_af_flr(dev, 1);
if (rc != -ENOTTY)
return rc;
In other-words, I can remove usage of pcie_has_flr() in all
other places in pci.c except in above function.
>
>>> I think we should keep the EXPORT_SYMBOL_GPL() part of a60a2b73ba69
>>> ("PCI: Export pcie_flr()"), but revert the restructuring part.
>>>
>>> Prior to a60a2b73ba69, we had
>>>
>>> int pcie_flr(struct pci_dev *dev, int probe);
>>>
>>> like all the other reset methods. AFAICT, the addition of
>>> pcie_has_flr() was to optimize the path slightly because when
>>> drivers call pcie_flr(), they should already know that their
>>> hardware supports FLR. But I don't think that optimization is
>>> worth the extra code complexity. If we do need to optimize it, we
>>> can check this in the core during enumeration and set
>>> PCI_DEV_FLAGS_NO_FLR_RESET accordingly.
>
>> Not all code paths are aware of FLR capability and also, not
>> using pcie_flr(). For example,
>>
>> arch/powerpc/platforms/powernv/eeh-powernv.c
>
> I assume you're referring to pnv_eeh_do_flr() (which contains code similar
> to pcie_flr()) and pnv_eeh_do_af_flr() (which has code similar to
> pci_af_flr()). I agree that those are problematic and would ideally be
> unified with the PCI core implementations.
>
> Powerpc has quite a bit of this sort of special-case code for several
> reasons, some just historical and some more concrete, so I don't know how
> feasible this is.
It would be lovely if pnv-eeh code used pci_af_flr() but since
pnv_eeh_do_flr() uses different config space accessors (not sure why
exactly, probably to avoid freezing the entire PHB), it is harder than just
trivial change. I'll try and have a deeper look though.
--
Alexey
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
Thanks Bjorn for your response. Please see below for my comments. So, we should consider one of these options. - set PCI_DEV_FLAGS_NO_FLR_RESET if it is not supported. - pcie_flr() should return if it is not supported If we modify pcie_flr() to return error codes, then we need to modify all existing modules that are calling this function. Yes, of course. Please let me know your preference, so that I can move accordingly. Thanks. I think Christoph volunteered to do some restructuring, but I don't know his timeframe. If you can, I would probably wait for that because there's so much overlap here. OK. The other paths that use PCI_EXP_DEVCTL_BCR_FLR are definitely issues and should be fixed, but again should wait for the revised pcie_flr() interface. And if they're not actually required for your Xen issue, they sound like "nice to have" cleanups that will not gate your Xen fixes. I added this to my ever-growing list of cleanups to do. For now, I am planning to use existing pcie_flr() after checking FLR capability inside Xenpciback driver (like other existing pcie_flr() usage). We will switch to revised pcie_flr() once it is available. Cheers GOVINDA
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
Thanks Bjorn for your response. Please see below for my comments. So, we should consider one of these options. - set PCI_DEV_FLAGS_NO_FLR_RESET if it is not supported. - pcie_flr() should return if it is not supported If we modify pcie_flr() to return error codes, then we need to modify all existing modules that are calling this function. Yes, of course. Please let me know your preference, so that I can move accordingly. Thanks. I think Christoph volunteered to do some restructuring, but I don't know his timeframe. If you can, I would probably wait for that because there's so much overlap here. OK. The other paths that use PCI_EXP_DEVCTL_BCR_FLR are definitely issues and should be fixed, but again should wait for the revised pcie_flr() interface. And if they're not actually required for your Xen issue, they sound like "nice to have" cleanups that will not gate your Xen fixes. I added this to my ever-growing list of cleanups to do. For now, I am planning to use existing pcie_flr() after checking FLR capability inside Xenpciback driver (like other existing pcie_flr() usage). We will switch to revised pcie_flr() once it is available. Cheers GOVINDA
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
[+cc Russell, Sinan, Herbert, Srikanth, Derek, Satanand, Felix, Raghu]
On Fri, Dec 15, 2017 at 09:48:02AM -0600, Govinda Tatti wrote:
> On 12/13/2017 3:24 PM, Bjorn Helgaas wrote:
> >On Wed, Dec 13, 2017 at 02:46:57PM -0600, Govinda Tatti wrote:
> >>-static bool pcie_has_flr(struct pci_dev *dev)
> >>+bool pcie_has_flr(struct pci_dev *dev)
> >> {
> >>u32 cap;
> >>@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev)
> >>pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
> >>return cap & PCI_EXP_DEVCAP_FLR;
> >> }
> >>+EXPORT_SYMBOL_GPL(pcie_has_flr);
> >I'd rather change pcie_flr() so you could *always* call it, and
> >it would return 0, -ENOTTY, or whatever, based on whether FLR
> >is supported. Is that feasible?
> Sure, I will add pcie_has_flr() logic inside pcie_flr() and
> return appropriate values as suggested by you. Do we still want
> to retain pcie_has_flr() and its usage inside pci.c?.Otherwise,
> I will remove it and do required cleanup.
> >>>If you can restructure the code and remove pcie_has_flr() while
> >>>retaining the existing behavior of its callers, that would be
> >>>great.
> >>I checked the current usage of pcie_has_flr() and pcie_flr(). I
> >>have a couple of questions or need some clarification.
> >>
> >>1. pcie_has_flr() usage inside pci_probe_reset_function().
> >>
> >> This function is only calling pcie_has_flr() but not pcie_flr().
> >> Rest of the code is trying to do specific type of reset except
> >>pcie_flr().
> >>
> >> rc = pci_dev_specific_reset(dev, 1);
> >> if (rc != -ENOTTY)
> >> return rc;
> >> if (pcie_has_flr(dev))
> >> return 0;
> >> rc = pci_af_flr(dev, 1);
> >> if (rc != -ENOTTY)
> >> return rc;
> >>
> >> In other-words, I can remove usage of pcie_has_flr() in all
> >>other places in pci.c except in above function.
> >I think we should keep the EXPORT_SYMBOL_GPL() part of a60a2b73ba69
> >("PCI: Export pcie_flr()"), but revert the restructuring part.
> >
> >Prior to a60a2b73ba69, we had
> >
> > int pcie_flr(struct pci_dev *dev, int probe);
> >
> >like all the other reset methods. AFAICT, the addition of
> >pcie_has_flr() was to optimize the path slightly because when
> >drivers call pcie_flr(), they should already know that their
> >hardware supports FLR. But I don't think that optimization is
> >worth the extra code complexity. If we do need to optimize it, we
> >can check this in the core during enumeration and set
> >PCI_DEV_FLAGS_NO_FLR_RESET accordingly.
> Not all code paths are aware of FLR capability and also, not
> using pcie_flr(). For example,
>
> arch/powerpc/platforms/powernv/eeh-powernv.c
I assume you're referring to pnv_eeh_do_flr() (which contains code similar
to pcie_flr()) and pnv_eeh_do_af_flr() (which has code similar to
pci_af_flr()). I agree that those are problematic and would ideally be
unified with the PCI core implementations.
Powerpc has quite a bit of this sort of special-case code for several
reasons, some just historical and some more concrete, so I don't know how
feasible this is.
> drivers/crypto/cavium/nitrox/nitrox_main.c
This has nitrox_reset_device(), which should definitely be replaced with a
core interface.
> drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c
And this has octeon_mbox_process_cmd() which also does a home-grown
PCI_EXP_DEVCTL_BCR_FLR request and also should definitely use a core
interface.
> So, we should consider one of these options.
>
> - set PCI_DEV_FLAGS_NO_FLR_RESET if it is not supported.
> - pcie_flr() should return if it is not supported
>
> If we modify pcie_flr() to return error codes, then we need to modify
> all existing modules that are calling this function.
Yes, of course.
> Please let me know your preference, so that I can move accordingly. Thanks.
I think Christoph volunteered to do some restructuring, but I don't
know his timeframe. If you can, I would probably wait for that
because there's so much overlap here.
The other paths that use PCI_EXP_DEVCTL_BCR_FLR are definitely issues
and should be fixed, but again should wait for the revised pcie_flr()
interface. And if they're not actually required for your Xen issue,
they sound like "nice to have" cleanups that will not gate your Xen
fixes. I added this to my ever-growing list of cleanups to do.
Bjorn
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
[+cc Russell, Sinan, Herbert, Srikanth, Derek, Satanand, Felix, Raghu]
On Fri, Dec 15, 2017 at 09:48:02AM -0600, Govinda Tatti wrote:
> On 12/13/2017 3:24 PM, Bjorn Helgaas wrote:
> >On Wed, Dec 13, 2017 at 02:46:57PM -0600, Govinda Tatti wrote:
> >>-static bool pcie_has_flr(struct pci_dev *dev)
> >>+bool pcie_has_flr(struct pci_dev *dev)
> >> {
> >>u32 cap;
> >>@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev)
> >>pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
> >>return cap & PCI_EXP_DEVCAP_FLR;
> >> }
> >>+EXPORT_SYMBOL_GPL(pcie_has_flr);
> >I'd rather change pcie_flr() so you could *always* call it, and
> >it would return 0, -ENOTTY, or whatever, based on whether FLR
> >is supported. Is that feasible?
> Sure, I will add pcie_has_flr() logic inside pcie_flr() and
> return appropriate values as suggested by you. Do we still want
> to retain pcie_has_flr() and its usage inside pci.c?.Otherwise,
> I will remove it and do required cleanup.
> >>>If you can restructure the code and remove pcie_has_flr() while
> >>>retaining the existing behavior of its callers, that would be
> >>>great.
> >>I checked the current usage of pcie_has_flr() and pcie_flr(). I
> >>have a couple of questions or need some clarification.
> >>
> >>1. pcie_has_flr() usage inside pci_probe_reset_function().
> >>
> >> This function is only calling pcie_has_flr() but not pcie_flr().
> >> Rest of the code is trying to do specific type of reset except
> >>pcie_flr().
> >>
> >> rc = pci_dev_specific_reset(dev, 1);
> >> if (rc != -ENOTTY)
> >> return rc;
> >> if (pcie_has_flr(dev))
> >> return 0;
> >> rc = pci_af_flr(dev, 1);
> >> if (rc != -ENOTTY)
> >> return rc;
> >>
> >> In other-words, I can remove usage of pcie_has_flr() in all
> >>other places in pci.c except in above function.
> >I think we should keep the EXPORT_SYMBOL_GPL() part of a60a2b73ba69
> >("PCI: Export pcie_flr()"), but revert the restructuring part.
> >
> >Prior to a60a2b73ba69, we had
> >
> > int pcie_flr(struct pci_dev *dev, int probe);
> >
> >like all the other reset methods. AFAICT, the addition of
> >pcie_has_flr() was to optimize the path slightly because when
> >drivers call pcie_flr(), they should already know that their
> >hardware supports FLR. But I don't think that optimization is
> >worth the extra code complexity. If we do need to optimize it, we
> >can check this in the core during enumeration and set
> >PCI_DEV_FLAGS_NO_FLR_RESET accordingly.
> Not all code paths are aware of FLR capability and also, not
> using pcie_flr(). For example,
>
> arch/powerpc/platforms/powernv/eeh-powernv.c
I assume you're referring to pnv_eeh_do_flr() (which contains code similar
to pcie_flr()) and pnv_eeh_do_af_flr() (which has code similar to
pci_af_flr()). I agree that those are problematic and would ideally be
unified with the PCI core implementations.
Powerpc has quite a bit of this sort of special-case code for several
reasons, some just historical and some more concrete, so I don't know how
feasible this is.
> drivers/crypto/cavium/nitrox/nitrox_main.c
This has nitrox_reset_device(), which should definitely be replaced with a
core interface.
> drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c
And this has octeon_mbox_process_cmd() which also does a home-grown
PCI_EXP_DEVCTL_BCR_FLR request and also should definitely use a core
interface.
> So, we should consider one of these options.
>
> - set PCI_DEV_FLAGS_NO_FLR_RESET if it is not supported.
> - pcie_flr() should return if it is not supported
>
> If we modify pcie_flr() to return error codes, then we need to modify
> all existing modules that are calling this function.
Yes, of course.
> Please let me know your preference, so that I can move accordingly. Thanks.
I think Christoph volunteered to do some restructuring, but I don't
know his timeframe. If you can, I would probably wait for that
because there's so much overlap here.
The other paths that use PCI_EXP_DEVCTL_BCR_FLR are definitely issues
and should be fixed, but again should wait for the revised pcie_flr()
interface. And if they're not actually required for your Xen issue,
they sound like "nice to have" cleanups that will not gate your Xen
fixes. I added this to my ever-growing list of cleanups to do.
Bjorn
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
Thanks Bjorn and Christophfor your response. Please see below for my
comments.
On 12/13/2017 3:24 PM, Bjorn Helgaas wrote:
[+cc Christoph]
On Wed, Dec 13, 2017 at 02:46:57PM -0600, Govinda Tatti wrote:
-static bool pcie_has_flr(struct pci_dev *dev)
+bool pcie_has_flr(struct pci_dev *dev)
{
u32 cap;
@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev)
pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
return cap & PCI_EXP_DEVCAP_FLR;
}
+EXPORT_SYMBOL_GPL(pcie_has_flr);
I'd rather change pcie_flr() so you could *always* call it, and it
would return 0, -ENOTTY, or whatever, based on whether FLR is
supported. Is that feasible?
Sure, I will add pcie_has_flr() logic inside pcie_flr() and return
appropriate
values as suggested by you. Do we still want to retain pcie_has_flr() and
its usage inside pci.c?.Otherwise, I will remove it and do required cleanup.
If you can restructure the code and remove pcie_has_flr() while
retaining the existing behavior of its callers, that would be great.
I checked the current usage of pcie_has_flr() and pcie_flr(). I have
a couple
of questions or need some clarification.
1. pcie_has_flr() usage inside pci_probe_reset_function().
This function is only calling pcie_has_flr() but not pcie_flr().
Rest of the code is trying to do specific type of reset except
pcie_flr().
rc = pci_dev_specific_reset(dev, 1);
if (rc != -ENOTTY)
return rc;
if (pcie_has_flr(dev))
return 0;
rc = pci_af_flr(dev, 1);
if (rc != -ENOTTY)
return rc;
In other-words, I can remove usage of pcie_has_flr() in all other places
in pci.c except in above function.
I think we should keep the EXPORT_SYMBOL_GPL() part of a60a2b73ba69
("PCI: Export pcie_flr()"), but revert the restructuring part.
Prior to a60a2b73ba69, we had
int pcie_flr(struct pci_dev *dev, int probe);
like all the other reset methods. AFAICT, the addition of
pcie_has_flr() was to optimize the path slightly because when drivers
call pcie_flr(), they should already know that their hardware supports
FLR. But I don't think that optimization is worth the extra code
complexity. If we do need to optimize it, we can check this in the
core during enumeration and set PCI_DEV_FLAGS_NO_FLR_RESET
accordingly.
Christoph, chime in if I'm missing something here.
Not all code paths are aware of FLR capability and also, not
using pcie_flr(). For example,
arch/powerpc/platforms/powernv/eeh-powernv.c
drivers/crypto/cavium/nitrox/nitrox_main.c
drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c
So, we should consider one of these options.
- set PCI_DEV_FLAGS_NO_FLR_RESET if it is not supported.
- pcie_flr() should return if it is not supported
If we modify pcie_flr() to return error codes, then we need to modify
all existing modules that are calling this function.
Please let me know your preference, so that I can move accordingly. Thanks.
Cheers
GOVINDA
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
Thanks Bjorn and Christophfor your response. Please see below for my
comments.
On 12/13/2017 3:24 PM, Bjorn Helgaas wrote:
[+cc Christoph]
On Wed, Dec 13, 2017 at 02:46:57PM -0600, Govinda Tatti wrote:
-static bool pcie_has_flr(struct pci_dev *dev)
+bool pcie_has_flr(struct pci_dev *dev)
{
u32 cap;
@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev)
pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
return cap & PCI_EXP_DEVCAP_FLR;
}
+EXPORT_SYMBOL_GPL(pcie_has_flr);
I'd rather change pcie_flr() so you could *always* call it, and it
would return 0, -ENOTTY, or whatever, based on whether FLR is
supported. Is that feasible?
Sure, I will add pcie_has_flr() logic inside pcie_flr() and return
appropriate
values as suggested by you. Do we still want to retain pcie_has_flr() and
its usage inside pci.c?.Otherwise, I will remove it and do required cleanup.
If you can restructure the code and remove pcie_has_flr() while
retaining the existing behavior of its callers, that would be great.
I checked the current usage of pcie_has_flr() and pcie_flr(). I have
a couple
of questions or need some clarification.
1. pcie_has_flr() usage inside pci_probe_reset_function().
This function is only calling pcie_has_flr() but not pcie_flr().
Rest of the code is trying to do specific type of reset except
pcie_flr().
rc = pci_dev_specific_reset(dev, 1);
if (rc != -ENOTTY)
return rc;
if (pcie_has_flr(dev))
return 0;
rc = pci_af_flr(dev, 1);
if (rc != -ENOTTY)
return rc;
In other-words, I can remove usage of pcie_has_flr() in all other places
in pci.c except in above function.
I think we should keep the EXPORT_SYMBOL_GPL() part of a60a2b73ba69
("PCI: Export pcie_flr()"), but revert the restructuring part.
Prior to a60a2b73ba69, we had
int pcie_flr(struct pci_dev *dev, int probe);
like all the other reset methods. AFAICT, the addition of
pcie_has_flr() was to optimize the path slightly because when drivers
call pcie_flr(), they should already know that their hardware supports
FLR. But I don't think that optimization is worth the extra code
complexity. If we do need to optimize it, we can check this in the
core during enumeration and set PCI_DEV_FLAGS_NO_FLR_RESET
accordingly.
Christoph, chime in if I'm missing something here.
Not all code paths are aware of FLR capability and also, not
using pcie_flr(). For example,
arch/powerpc/platforms/powernv/eeh-powernv.c
drivers/crypto/cavium/nitrox/nitrox_main.c
drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c
So, we should consider one of these options.
- set PCI_DEV_FLAGS_NO_FLR_RESET if it is not supported.
- pcie_flr() should return if it is not supported
If we modify pcie_flr() to return error codes, then we need to modify
all existing modules that are calling this function.
Please let me know your preference, so that I can move accordingly. Thanks.
Cheers
GOVINDA
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
On Thu, Dec 14, 2017 at 04:52:06AM -0800, Christoph Hellwig wrote: > On Wed, Dec 13, 2017 at 03:24:21PM -0600, Bjorn Helgaas wrote: > > Prior to a60a2b73ba69, we had > > > > int pcie_flr(struct pci_dev *dev, int probe); > > > > like all the other reset methods. AFAICT, the addition of > > pcie_has_flr() was to optimize the path slightly because when drivers > > call pcie_flr(), they should already know that their hardware supports > > FLR. But I don't think that optimization is worth the extra code > > complexity. If we do need to optimize it, we can check this in the > > core during enumeration and set PCI_DEV_FLAGS_NO_FLR_RESET > > accordingly. > > > > Christoph, chime in if I'm missing something here. > > Didn't we just have that discussion in another thread a few days > ago? Probably, but I didn't have a clear picture of what you were suggesting. > I think that the pcie_has_flr was a mistake in retrospective but I > think the bool probe API was an even bigger mistake. The only use > of it is to hide the reset attribute in sysfs. I'd much rather always > have it and have it return EOPNOTSUPP if no reset method is supported. > > I can send a patch for that if it sounds fine to you. If you can get rid of the whole probe infrastructure, that sounds good to me. Bjorn
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
On Thu, Dec 14, 2017 at 04:52:06AM -0800, Christoph Hellwig wrote: > On Wed, Dec 13, 2017 at 03:24:21PM -0600, Bjorn Helgaas wrote: > > Prior to a60a2b73ba69, we had > > > > int pcie_flr(struct pci_dev *dev, int probe); > > > > like all the other reset methods. AFAICT, the addition of > > pcie_has_flr() was to optimize the path slightly because when drivers > > call pcie_flr(), they should already know that their hardware supports > > FLR. But I don't think that optimization is worth the extra code > > complexity. If we do need to optimize it, we can check this in the > > core during enumeration and set PCI_DEV_FLAGS_NO_FLR_RESET > > accordingly. > > > > Christoph, chime in if I'm missing something here. > > Didn't we just have that discussion in another thread a few days > ago? Probably, but I didn't have a clear picture of what you were suggesting. > I think that the pcie_has_flr was a mistake in retrospective but I > think the bool probe API was an even bigger mistake. The only use > of it is to hide the reset attribute in sysfs. I'd much rather always > have it and have it return EOPNOTSUPP if no reset method is supported. > > I can send a patch for that if it sounds fine to you. If you can get rid of the whole probe infrastructure, that sounds good to me. Bjorn
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
On Wed, Dec 13, 2017 at 03:24:21PM -0600, Bjorn Helgaas wrote: > Prior to a60a2b73ba69, we had > > int pcie_flr(struct pci_dev *dev, int probe); > > like all the other reset methods. AFAICT, the addition of > pcie_has_flr() was to optimize the path slightly because when drivers > call pcie_flr(), they should already know that their hardware supports > FLR. But I don't think that optimization is worth the extra code > complexity. If we do need to optimize it, we can check this in the > core during enumeration and set PCI_DEV_FLAGS_NO_FLR_RESET > accordingly. > > Christoph, chime in if I'm missing something here. Didn't we just have that discussion in another thread a few days ago? I think that the pcie_has_flr was a mistake in retrospective but I think the bool probe API was an even bigger mistake. The only use of it is to hide the reset attribute in sysfs. I'd much rather always have it and have it return EOPNOTSUPP if no reset method is supported. I can send a patch for that if it sounds fine to you.
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
On Wed, Dec 13, 2017 at 03:24:21PM -0600, Bjorn Helgaas wrote: > Prior to a60a2b73ba69, we had > > int pcie_flr(struct pci_dev *dev, int probe); > > like all the other reset methods. AFAICT, the addition of > pcie_has_flr() was to optimize the path slightly because when drivers > call pcie_flr(), they should already know that their hardware supports > FLR. But I don't think that optimization is worth the extra code > complexity. If we do need to optimize it, we can check this in the > core during enumeration and set PCI_DEV_FLAGS_NO_FLR_RESET > accordingly. > > Christoph, chime in if I'm missing something here. Didn't we just have that discussion in another thread a few days ago? I think that the pcie_has_flr was a mistake in retrospective but I think the bool probe API was an even bigger mistake. The only use of it is to hide the reset attribute in sysfs. I'd much rather always have it and have it return EOPNOTSUPP if no reset method is supported. I can send a patch for that if it sounds fine to you.
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
[+cc Christoph]
On Wed, Dec 13, 2017 at 02:46:57PM -0600, Govinda Tatti wrote:
>
> -static bool pcie_has_flr(struct pci_dev *dev)
> +bool pcie_has_flr(struct pci_dev *dev)
> {
> u32 cap;
> @@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev)
> pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
> return cap & PCI_EXP_DEVCAP_FLR;
> }
> +EXPORT_SYMBOL_GPL(pcie_has_flr);
> >>>I'd rather change pcie_flr() so you could *always* call it, and it
> >>>would return 0, -ENOTTY, or whatever, based on whether FLR is
> >>>supported. Is that feasible?
> >>Sure, I will add pcie_has_flr() logic inside pcie_flr() and return
> >>appropriate
> >>values as suggested by you. Do we still want to retain pcie_has_flr() and
> >>its usage inside pci.c?.Otherwise, I will remove it and do required cleanup.
> >If you can restructure the code and remove pcie_has_flr() while
> >retaining the existing behavior of its callers, that would be great.
> I checked the current usage of pcie_has_flr() and pcie_flr(). I have
> a couple
> of questions or need some clarification.
>
> 1. pcie_has_flr() usage inside pci_probe_reset_function().
>
> This function is only calling pcie_has_flr() but not pcie_flr().
> Rest of the code is trying to do specific type of reset except
> pcie_flr().
>
> rc = pci_dev_specific_reset(dev, 1);
> if (rc != -ENOTTY)
> return rc;
> if (pcie_has_flr(dev))
> return 0;
> rc = pci_af_flr(dev, 1);
> if (rc != -ENOTTY)
> return rc;
>
> In other-words, I can remove usage of pcie_has_flr() in all other places
> in pci.c except in above function.
I think we should keep the EXPORT_SYMBOL_GPL() part of a60a2b73ba69
("PCI: Export pcie_flr()"), but revert the restructuring part.
Prior to a60a2b73ba69, we had
int pcie_flr(struct pci_dev *dev, int probe);
like all the other reset methods. AFAICT, the addition of
pcie_has_flr() was to optimize the path slightly because when drivers
call pcie_flr(), they should already know that their hardware supports
FLR. But I don't think that optimization is worth the extra code
complexity. If we do need to optimize it, we can check this in the
core during enumeration and set PCI_DEV_FLAGS_NO_FLR_RESET
accordingly.
Christoph, chime in if I'm missing something here.
> 2. W.r.t pcie_flr(), I am planning to return error code. Currently,
> the following
> file/modules are calling this function. My plan is to add a check
> for return
> code and print a WANRING message if return code is NON-ZERO. I
> hope this is
> sufficient for this patch.
>
> drivers/crypto/qat/qat_common/adf_aer.c
> drivers/infiniband/hw/hfi1/chip.c (2 places)
> drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c (2 places)
> drivers/pci/quirks.c (2 places)
Checking the return code is probably overkill, since pcie_flr() is
void and returns no errors now. The only point of the return value is
to tell whether the device supports FLR. If we call it with "probe ==
0" there's no useful error to return.
Bjorn
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
[+cc Christoph]
On Wed, Dec 13, 2017 at 02:46:57PM -0600, Govinda Tatti wrote:
>
> -static bool pcie_has_flr(struct pci_dev *dev)
> +bool pcie_has_flr(struct pci_dev *dev)
> {
> u32 cap;
> @@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev)
> pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
> return cap & PCI_EXP_DEVCAP_FLR;
> }
> +EXPORT_SYMBOL_GPL(pcie_has_flr);
> >>>I'd rather change pcie_flr() so you could *always* call it, and it
> >>>would return 0, -ENOTTY, or whatever, based on whether FLR is
> >>>supported. Is that feasible?
> >>Sure, I will add pcie_has_flr() logic inside pcie_flr() and return
> >>appropriate
> >>values as suggested by you. Do we still want to retain pcie_has_flr() and
> >>its usage inside pci.c?.Otherwise, I will remove it and do required cleanup.
> >If you can restructure the code and remove pcie_has_flr() while
> >retaining the existing behavior of its callers, that would be great.
> I checked the current usage of pcie_has_flr() and pcie_flr(). I have
> a couple
> of questions or need some clarification.
>
> 1. pcie_has_flr() usage inside pci_probe_reset_function().
>
> This function is only calling pcie_has_flr() but not pcie_flr().
> Rest of the code is trying to do specific type of reset except
> pcie_flr().
>
> rc = pci_dev_specific_reset(dev, 1);
> if (rc != -ENOTTY)
> return rc;
> if (pcie_has_flr(dev))
> return 0;
> rc = pci_af_flr(dev, 1);
> if (rc != -ENOTTY)
> return rc;
>
> In other-words, I can remove usage of pcie_has_flr() in all other places
> in pci.c except in above function.
I think we should keep the EXPORT_SYMBOL_GPL() part of a60a2b73ba69
("PCI: Export pcie_flr()"), but revert the restructuring part.
Prior to a60a2b73ba69, we had
int pcie_flr(struct pci_dev *dev, int probe);
like all the other reset methods. AFAICT, the addition of
pcie_has_flr() was to optimize the path slightly because when drivers
call pcie_flr(), they should already know that their hardware supports
FLR. But I don't think that optimization is worth the extra code
complexity. If we do need to optimize it, we can check this in the
core during enumeration and set PCI_DEV_FLAGS_NO_FLR_RESET
accordingly.
Christoph, chime in if I'm missing something here.
> 2. W.r.t pcie_flr(), I am planning to return error code. Currently,
> the following
> file/modules are calling this function. My plan is to add a check
> for return
> code and print a WANRING message if return code is NON-ZERO. I
> hope this is
> sufficient for this patch.
>
> drivers/crypto/qat/qat_common/adf_aer.c
> drivers/infiniband/hw/hfi1/chip.c (2 places)
> drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c (2 places)
> drivers/pci/quirks.c (2 places)
Checking the return code is probably overkill, since pcie_flr() is
void and returns no errors now. The only point of the return value is
to tell whether the device supports FLR. If we call it with "probe ==
0" there's no useful error to return.
Bjorn
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
-static bool pcie_has_flr(struct pci_dev *dev)
+bool pcie_has_flr(struct pci_dev *dev)
{
u32 cap;
@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev)
pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
return cap & PCI_EXP_DEVCAP_FLR;
}
+EXPORT_SYMBOL_GPL(pcie_has_flr);
I'd rather change pcie_flr() so you could *always* call it, and it
would return 0, -ENOTTY, or whatever, based on whether FLR is
supported. Is that feasible?
Sure, I will add pcie_has_flr() logic inside pcie_flr() and return
appropriate
values as suggested by you. Do we still want to retain pcie_has_flr() and
its usage inside pci.c?.Otherwise, I will remove it and do required cleanup.
If you can restructure the code and remove pcie_has_flr() while
retaining the existing behavior of its callers, that would be great.
I checked the current usage of pcie_has_flr() and pcie_flr(). I have a
couple
of questions or need some clarification.
1. pcie_has_flr() usage inside pci_probe_reset_function().
This function is only calling pcie_has_flr() but not pcie_flr().
Rest of the code is trying to do specific type of reset except
pcie_flr().
rc = pci_dev_specific_reset(dev, 1);
if (rc != -ENOTTY)
return rc;
if (pcie_has_flr(dev))
return 0;
rc = pci_af_flr(dev, 1);
if (rc != -ENOTTY)
return rc;
In other-words, I can remove usage of pcie_has_flr() in all other places
in pci.c except in above function.
2. W.r.t pcie_flr(), I am planning to return error code. Currently, the
following
file/modules are calling this function. My plan is to add a check
for return
code and print a WANRING message if return code is NON-ZERO. I hope
this is
sufficient for this patch.
drivers/crypto/qat/qat_common/adf_aer.c
drivers/infiniband/hw/hfi1/chip.c (2 places)
drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c (2 places)
drivers/pci/quirks.c (2 places)
Cheers
GOVINDA
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
-static bool pcie_has_flr(struct pci_dev *dev)
+bool pcie_has_flr(struct pci_dev *dev)
{
u32 cap;
@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev)
pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, );
return cap & PCI_EXP_DEVCAP_FLR;
}
+EXPORT_SYMBOL_GPL(pcie_has_flr);
I'd rather change pcie_flr() so you could *always* call it, and it
would return 0, -ENOTTY, or whatever, based on whether FLR is
supported. Is that feasible?
Sure, I will add pcie_has_flr() logic inside pcie_flr() and return
appropriate
values as suggested by you. Do we still want to retain pcie_has_flr() and
its usage inside pci.c?.Otherwise, I will remove it and do required cleanup.
If you can restructure the code and remove pcie_has_flr() while
retaining the existing behavior of its callers, that would be great.
I checked the current usage of pcie_has_flr() and pcie_flr(). I have a
couple
of questions or need some clarification.
1. pcie_has_flr() usage inside pci_probe_reset_function().
This function is only calling pcie_has_flr() but not pcie_flr().
Rest of the code is trying to do specific type of reset except
pcie_flr().
rc = pci_dev_specific_reset(dev, 1);
if (rc != -ENOTTY)
return rc;
if (pcie_has_flr(dev))
return 0;
rc = pci_af_flr(dev, 1);
if (rc != -ENOTTY)
return rc;
In other-words, I can remove usage of pcie_has_flr() in all other places
in pci.c except in above function.
2. W.r.t pcie_flr(), I am planning to return error code. Currently, the
following
file/modules are calling this function. My plan is to add a check
for return
code and print a WANRING message if return code is NON-ZERO. I hope
this is
sufficient for this patch.
drivers/crypto/qat/qat_common/adf_aer.c
drivers/infiniband/hw/hfi1/chip.c (2 places)
drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c (2 places)
drivers/pci/quirks.c (2 places)
Cheers
GOVINDA
