On Mon, Mar 31, 2014 at 04:51:53PM -0700, Rajat Jain wrote:
> Today, there is a global pciehp_poll_mode module parameter using which
> either _all_ the hot-pluggable ports are to use polling, or _all_ the
> ports are to use interrupts.
>
> In a system where a certain port has IRQ issues, today the only option
> is to use the parameter that converts ALL the ports to use polling mode.
> This is not good, and hence this patch intruduces a bit field that can
> be set using a PCI quirk that indicates that polling should always be
> used for this particular PCIe port. The remaining ports can still
> hoose to continue to operate in whatever mode they wish to.
>
> Signed-off-by: Rajat Jain <[email protected]>
> Signed-off-by: Rajat Jain <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
I'm willing to merge this, but I'd prefer to merge it along with a quirk
that actually sets dev->hotplug_polling. Otherwise it's dead code and I'll
have no way to tell whether we need to keep it.
Bjorn
> ---
> drivers/pci/hotplug/pciehp.h | 1 +
> drivers/pci/hotplug/pciehp_hpc.c | 16 ++++++++++------
> include/linux/pci.h | 1 +
> 3 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index d208791..753a3b4 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -98,6 +98,7 @@ struct controller {
> unsigned int no_cmd_complete:1;
> unsigned int link_active_reporting:1;
> unsigned int notification_enabled:1;
> + unsigned int use_polling:1; /* Always uses polling for this slot */
> unsigned int power_fault_detected;
> };
>
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index d7d058f..d210d23 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -82,7 +82,7 @@ static inline int pciehp_request_irq(struct controller *ctrl)
> int retval, irq = ctrl->pcie->irq;
>
> /* Install interrupt polling timer. Start with 10 sec delay */
> - if (pciehp_poll_mode) {
> + if (ctrl->use_polling) {
> init_timer(&ctrl->poll_timer);
> start_int_poll_timer(ctrl, 10);
> return 0;
> @@ -98,7 +98,7 @@ static inline int pciehp_request_irq(struct controller *ctrl)
>
> static inline void pciehp_free_irq(struct controller *ctrl)
> {
> - if (pciehp_poll_mode)
> + if (ctrl->use_polling)
> del_timer_sync(&ctrl->poll_timer);
> else
> free_irq(ctrl->pcie->irq, ctrl);
> @@ -131,7 +131,7 @@ static int pcie_poll_cmd(struct controller *ctrl)
>
> static void pcie_wait_cmd(struct controller *ctrl, int poll)
> {
> - unsigned int msecs = pciehp_poll_mode ? 2500 : 1000;
> + unsigned int msecs = ctrl->use_polling ? 2500 : 1000;
> unsigned long timeout = msecs_to_jiffies(msecs);
> int rc;
>
> @@ -595,7 +595,7 @@ void pcie_enable_notification(struct controller *ctrl)
> cmd |= PCI_EXP_SLTCTL_PDCE;
> if (MRL_SENS(ctrl))
> cmd |= PCI_EXP_SLTCTL_MRLSCE;
> - if (!pciehp_poll_mode)
> + if (!ctrl->use_polling)
> cmd |= PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE;
>
> mask = (PCI_EXP_SLTCTL_PDCE | PCI_EXP_SLTCTL_ABPE |
> @@ -642,14 +642,14 @@ int pciehp_reset_slot(struct slot *slot, int probe)
> stat_mask |= PCI_EXP_SLTSTA_DLLSC;
>
> pcie_write_cmd(ctrl, 0, ctrl_mask);
> - if (pciehp_poll_mode)
> + if (ctrl->use_polling)
> del_timer_sync(&ctrl->poll_timer);
>
> pci_reset_bridge_secondary_bus(ctrl->pcie->port);
>
> pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, stat_mask);
> pcie_write_cmd(ctrl, ctrl_mask, ctrl_mask);
> - if (pciehp_poll_mode)
> + if (ctrl->use_polling)
> int_poll_timeout(ctrl->poll_timer.data);
>
> return 0;
> @@ -789,6 +789,10 @@ struct controller *pcie_init(struct pcie_device *dev)
> ctrl_dbg(ctrl, "Link Active Reporting supported\n");
> ctrl->link_active_reporting = 1;
> }
> + if (pciehp_poll_mode || dev->port->hotplug_polling) {
> + ctrl_info(ctrl, "will use polling\n");
> + ctrl->use_polling = 1;
> + }
>
> /* Clear all remaining event bits in Slot Status register */
> pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index a13d682..b2ec72e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -336,6 +336,7 @@ struct pci_dev {
> unsigned int is_virtfn:1;
> unsigned int reset_fn:1;
> unsigned int is_hotplug_bridge:1;
> + unsigned int hotplug_polling:1; /* Port uses polling for hotplug */
> unsigned int __aer_firmware_first_valid:1;
> unsigned int __aer_firmware_first:1;
> unsigned int broken_intx_masking:1;
> --
> 1.7.9.5
>
> -----Original Message-----
> From: Bjorn Helgaas [mailto:[email protected]]
> Sent: Thursday, April 24, 2014 2:31 PM
> To: Rajat Jain
> Cc: [email protected]; [email protected]; Rajat
> Jain; Guenter Roeck
> Subject: Re: [PATCH] pci/pciehp: Allow polling/irq mode to be decided
> on a per-port basis
>
> On Mon, Mar 31, 2014 at 04:51:53PM -0700, Rajat Jain wrote:
> > Today, there is a global pciehp_poll_mode module parameter using
> which
> > either _all_ the hot-pluggable ports are to use polling, or _all_ the
> > ports are to use interrupts.
> >
> > In a system where a certain port has IRQ issues, today the only
> option
> > is to use the parameter that converts ALL the ports to use polling
> mode.
> > This is not good, and hence this patch intruduces a bit field that
> can
> > be set using a PCI quirk that indicates that polling should always be
> > used for this particular PCIe port. The remaining ports can still
> > hoose to continue to operate in whatever mode they wish to.
> >
> > Signed-off-by: Rajat Jain <[email protected]>
> > Signed-off-by: Rajat Jain <[email protected]>
> > Signed-off-by: Guenter Roeck <[email protected]>
>
> I'm willing to merge this, but I'd prefer to merge it along with a
> quirk that actually sets dev->hotplug_polling. Otherwise it's dead
> code and I'll have no way to tell whether we need to keep it.
>
Bjorn,
what would be the proper location for such a quirk ?
We use it to help simulating hotplug support on an IDT PES12NT3.
The code is a bit more invasive than just the quirk itself,
since it also needs to touch link and slot status registers,
so quirks.c doesn't seem appropriate.
drivers/pci/pes12nt3.c, maybe, with a separate configuration
option ? Or in the hotplug directory ?
Guenter
On Fri, Apr 25, 2014 at 10:34 AM, Guenter Roeck <[email protected]> wrote:
>
>
>> -----Original Message-----
>> From: Bjorn Helgaas [mailto:[email protected]]
>> Sent: Thursday, April 24, 2014 2:31 PM
>> To: Rajat Jain
>> Cc: [email protected]; [email protected]; Rajat
>> Jain; Guenter Roeck
>> Subject: Re: [PATCH] pci/pciehp: Allow polling/irq mode to be decided
>> on a per-port basis
>>
>> On Mon, Mar 31, 2014 at 04:51:53PM -0700, Rajat Jain wrote:
>> > Today, there is a global pciehp_poll_mode module parameter using
>> which
>> > either _all_ the hot-pluggable ports are to use polling, or _all_ the
>> > ports are to use interrupts.
>> >
>> > In a system where a certain port has IRQ issues, today the only
>> option
>> > is to use the parameter that converts ALL the ports to use polling
>> mode.
>> > This is not good, and hence this patch intruduces a bit field that
>> can
>> > be set using a PCI quirk that indicates that polling should always be
>> > used for this particular PCIe port. The remaining ports can still
>> > hoose to continue to operate in whatever mode they wish to.
>> >
>> > Signed-off-by: Rajat Jain <[email protected]>
>> > Signed-off-by: Rajat Jain <[email protected]>
>> > Signed-off-by: Guenter Roeck <[email protected]>
>>
>> I'm willing to merge this, but I'd prefer to merge it along with a
>> quirk that actually sets dev->hotplug_polling. Otherwise it's dead
>> code and I'll have no way to tell whether we need to keep it.
>>
> Bjorn,
>
> what would be the proper location for such a quirk ?
> We use it to help simulating hotplug support on an IDT PES12NT3.
> The code is a bit more invasive than just the quirk itself,
> since it also needs to touch link and slot status registers,
> so quirks.c doesn't seem appropriate.
>
> drivers/pci/pes12nt3.c, maybe, with a separate configuration
> option ? Or in the hotplug directory ?
If this is only for debug, i.e., you don't intend to ship a product
using this simulated hotplug, maybe you should just keep both the
quirk and this patch out of tree.
If you do want to eventually ship this code for some product, I think
it'd be fine to put the quirk in drivers/pci/quirks.c, maybe with a
config option to enable it. But without seeing the quirk, I can't
really tell. A new file seems overkill unless it's something really
huge -- I don't think we really have examples of dedicated files for
other chip idiosyncrasies.
Bjorn
> -----Original Message-----
> From: Bjorn Helgaas [mailto:[email protected]]
> Sent: Friday, April 25, 2014 9:44 AM
> To: Guenter Roeck
> Cc: Rajat Jain; [email protected]; linux-
> [email protected]; Rajat Jain
> Subject: Re: [PATCH] pci/pciehp: Allow polling/irq mode to be decided
> on a per-port basis
>
> On Fri, Apr 25, 2014 at 10:34 AM, Guenter Roeck <[email protected]>
> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Bjorn Helgaas [mailto:[email protected]]
> >> Sent: Thursday, April 24, 2014 2:31 PM
> >> To: Rajat Jain
> >> Cc: [email protected]; [email protected]; Rajat
> >> Jain; Guenter Roeck
> >> Subject: Re: [PATCH] pci/pciehp: Allow polling/irq mode to be
> decided
> >> on a per-port basis
> >>
> >> On Mon, Mar 31, 2014 at 04:51:53PM -0700, Rajat Jain wrote:
> >> > Today, there is a global pciehp_poll_mode module parameter using
> >> which
> >> > either _all_ the hot-pluggable ports are to use polling, or _all_
> >> > the ports are to use interrupts.
> >> >
> >> > In a system where a certain port has IRQ issues, today the only
> >> option
> >> > is to use the parameter that converts ALL the ports to use polling
> >> mode.
> >> > This is not good, and hence this patch intruduces a bit field that
> >> can
> >> > be set using a PCI quirk that indicates that polling should always
> >> > be used for this particular PCIe port. The remaining ports can
> >> > still hoose to continue to operate in whatever mode they wish to.
> >> >
> >> > Signed-off-by: Rajat Jain <[email protected]>
> >> > Signed-off-by: Rajat Jain <[email protected]>
> >> > Signed-off-by: Guenter Roeck <[email protected]>
> >>
> >> I'm willing to merge this, but I'd prefer to merge it along with a
> >> quirk that actually sets dev->hotplug_polling. Otherwise it's dead
> >> code and I'll have no way to tell whether we need to keep it.
> >>
> > Bjorn,
> >
> > what would be the proper location for such a quirk ?
> > We use it to help simulating hotplug support on an IDT PES12NT3.
> > The code is a bit more invasive than just the quirk itself, since it
> > also needs to touch link and slot status registers, so quirks.c
> > doesn't seem appropriate.
> >
> > drivers/pci/pes12nt3.c, maybe, with a separate configuration option ?
> > Or in the hotplug directory ?
>
> If this is only for debug, i.e., you don't intend to ship a product
> using this simulated hotplug, maybe you should just keep both the quirk
> and this patch out of tree.
>
> If you do want to eventually ship this code for some product, I think
> it'd be fine to put the quirk in drivers/pci/quirks.c, maybe with a
> config option to enable it. But without seeing the quirk, I can't
> really tell. A new file seems overkill unless it's something really
> huge -- I don't think we really have examples of dedicated files for
> other chip idiosyncrasies.
>
I'd give it a 50:50 probability that it will ship. Current plan is that
it is for development only, but I suspect that may change at some point.
I agree, this is kind of an outlier. If we push it upstream, it might
mostly serve as a reference for others who might have similar problems -
not just for the quirk itself, but as an example on how to intercept
and manipulate pci configuration register accesses.
I attached the file so you can have a look.
Guenter