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

[Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>]

(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