[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-hotplug
Subject:    Re: [PATCH] Set PCIE maxpayload for card during hotplug insertion
From:       Jesse Barnes <jbarnes () virtuousgeek ! org>
Date:       2011-04-12 16:08:08
Message-ID: 20110412090808.5bdf9945 () jbarnes-desktop
[Download RAW message or body]

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=128932324625063&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
--
To unsubscribe from this list: send the line "unsubscribe linux-hotplug" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[prev in list] [next in list] [prev in thread] [next in thread]