Re: [PATCH] Set PCIE maxpayload for card during hotplug insertion

[Jesse Barnes <jbarnes@virtuousgeek.org>]

Kenji-san, does the most recent version look ok?  An added tested-by to
confirm the payload size is set correctly would be a bonus.

Thanks,
Jesse

On Tue, 29 Mar 2011 16:42:25 -0500
<Jordan_Hargrave@Dell.com> wrote:

> Are there any further comments on this diff?
> 
> Would it be better to have a separate pcie_get_maxpayload/pcie_set_maxpayload function? 
> 
> What about cards that fail the 'if (psz > dmax)' test?  Technically the code should walk up to the root complex and set the speed there to the least common denominator speed of all children.  However if that happens, the performance of other cards/PCI devices in the system maybe adversely affected.  Any ideas on how to implement this or if this is desired?
> 
> --jordan hargrave
> Dell Enterprise Linux Engineering
> 
> > -----Original Message-----
> > From: Hargrave, Jordan
> > Sent: Monday, March 28, 2011 2:53 AM
> > Cc: 'linux-hotplug@vger.kernel.org'; 'linux-pci@vger.kernel.org'
> > Subject: RE: [PATCH] Set PCIE maxpayload for card during hotplug
> > insertion
> > 
> > > -----Original Message-----
> > > From: Kenji Kaneshige [mailto:kaneshige.kenji@jp.fujitsu.com]
> > > Sent: Thursday, March 24, 2011 8:07 PM
> > > To: Hargrave, Jordan
> > > Cc: linux-hotplug@vger.kernel.org; linux-pci@vger.kernel.org;
> > > jbarnes@virtuousgeek.org
> > > Subject: Re: [PATCH] Set PCIE maxpayload for card during hotplug
> > > insertion
> > >
> > > (2011/03/25 7:21), Jordan Hargrave wrote:
> > > > Reference: http://marc.info/?l=linux-hotplug&m\x128932324625063&w=2
> > > >
> > > > The following patch sets the MaxPayload setting to match the parent
> > > reading when inserting
> > > > a PCIE card into a hotplug slot.  On our system, the upstream
> > bridge
> > > is set to 256, but when
> > > > inserting a card, the card setting defaults to 128.  As soon as I/O
> > > is performed to the card
> > > > it starts receiving errors since the payload size is too small.
> > > >
> > > > Signed-off-by: Jordan_Hargrave@dell.com
> > > >
> > > > ---
> > > >   drivers/pci/hotplug/pcihp_slot.c |   45
> > > ++++++++++++++++++++++++++++++++++++++
> > > >   1 files changed, 45 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/hotplug/pcihp_slot.c
> > > b/drivers/pci/hotplug/pcihp_slot.c
> > > > index 80b461c..912d55b 100644
> > > > --- a/drivers/pci/hotplug/pcihp_slot.c
> > > > +++ b/drivers/pci/hotplug/pcihp_slot.c
> > > > @@ -158,6 +158,47 @@ static void program_hpp_type2(struct pci_dev
> > > *dev, struct hpp_type2 *hpp)
> > > >   	 */
> > > >   }
> > > >
> > > > +/* Program PCIE MaxPayload setting on device: ensure parent
> > > maxpayload<= device */
> > > > +static int pci_set_payload(struct pci_dev *dev)
> > > > +{
> > > > +	int pos, ppos;
> > > > +	u16 pctl, psz;
> > > > +	u16 dctl, dsz, dcap, dmax;
> > > > +	struct pci_dev *parent;
> > > > +
> > > > +	parent = dev->bus->self;
> > > > +	pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
> > > > +	if (!pos)
> > > > +		return 0;
> > >
> > > You can use pci_pcie_cap(), which just returns dev->pcie_cap, instead
> > > of pci_find_capability().
> > >
> > > > +
> > > > +	/* Read Device MaxPayload capability and setting */
> > > > +	pci_read_config_word(dev, pos + PCI_EXP_DEVCTL,&dctl);
> > > > +	pci_read_config_word(dev, pos + PCI_EXP_DEVCAP,&dcap);
> > > > +	dsz = (dctl&  PCI_EXP_DEVCTL_PAYLOAD)>>  5;
> > > > +	dmax = (dcap&  PCI_EXP_DEVCAP_PAYLOAD);
> > > > +
> > > > +	/* Read Parent MaxPayload setting */
> > > > +	ppos = pci_find_capability(parent, PCI_CAP_ID_EXP);
> > > > +	if (!ppos)
> > > > +		return 0;
> > >
> > > Ditto.
> > >
> > > > +	pci_read_config_word(parent, ppos + PCI_EXP_DEVCTL,&pctl);
> > > > +	psz = (pctl&   PCI_EXP_DEVCTL_PAYLOAD)>>  5;
> > > > +
> > > > +	/* If parent payload>  device max payload ->  error
> > > > +	 * If parent payload>  device payload ->  set speed
> > > > +	 * If parent payload<= device payload ->  do nothing
> > > > +	 */
> > > > +	if (psz>  dmax)
> > > > +		return -1;
> > > > +	else if (psz>  dsz) {
> > >
> > > Maybe just "if" (not "else if") here is easier to read.
> > >
> > > > +		dev_info(&dev->dev, "Setting MaxPayload to %d\n", 128<<
> > > psz);
> > > > +		pci_write_config_word(dev, pos + PCI_EXP_DEVCTL,
> > > > +				      (dctl&  ~PCI_EXP_DEVCTL_PAYLOAD) +
> > > > +				      (psz<<  5));
> > >
> > > What about
> > >
> > > 	dctrl = (dctrl & ~PCI_EXP_DEVCTL_PAYLOAD) | (psz << 5);
> > > 	pci_write_config_word(dev, pos + PCI_EXP_DEVCTL, dctrl);
> > >
> > > ?
> > >
> > > Regards,
> > > 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
> 


-- 
Jesse Barnes, Intel Open Source Technology Center