On Mon, Sep 26, 2011 at 3:38 PM, Benjamin Herrenschmidt
<benh@xxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, 2011-09-26 at 09:40 -0500, Jon Mason wrote:
>
>> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> > index 4e84fd4..d369316 100644
>> > --- a/drivers/pci/pci.c
>> > +++ b/drivers/pci/pci.c
>> > @@ -3203,8 +3203,6 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
>> > if (rq < 128 || rq > 4096 || !is_power_of_2(rq))
>> > goto out;
>> >
>> > - v = (ffs(rq) - 8) << 12;
>> > -
>> > cap = pci_pcie_cap(dev);
>> > if (!cap)
>> > goto out;
>> > @@ -3212,6 +3210,22 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
>> > err = pci_read_config_word(dev, cap + PCI_EXP_DEVCTL, &ctl);
>> > if (err)
>> > goto out;
>> > + /*
>> > + * If using the "performance" PCIe config, we clamp the
>> > + * read rq size to the max packet size to prevent the
>> > + * host bridge generating requests larger than we can
>> > + * cope with
>> > + */
>> > + if (pcie_bus_config == PCIE_BUS_PERFORMANCE) {
>> > + int mps = pcie_get_mps(dev);
>> > +
>> > + if (mps < 0)
>> > + return mps;
>>
>> Shouldn't this return an error, not the mps? "goto out" is what I was using.
>
> There is no lock to release or anything like that so goto isn't useful,
> return is fine. As for mps, well, mps < 0 -is- an error :-) We just pass
> up the error code we got from pcie_get_mps()
Doh! Guess I should finish my morning coffee before responding.
>
>> > + if (mps < rq)
>> > + rq = mps;
>>
>> I had a dev_warn in there to notify the user.
>
> I'm not sure we really need it but I can add it. Maybe a dev_dbg() tho ?
> The "user" in 99% of the time has no idea and there's nothing actually
> wrong here.
My worry is that this modification of the driver request for a larger
MRRS will be hidden, and someone will spend time trying to figure out
why they aren't getting what they asked for. I suppose this should be
tempered by the want of everyone else to have a less noisy dmesg.
dev_dbg is a good compromise.
Thanks,
Jon
>
> Cheers,
> Ben.
>
>> > + }
>> > +
>> > + v = (ffs(rq) - 8) << 12;
>> >
>> > if ((ctl & PCI_EXP_DEVCTL_READRQ) != v) {
>> > ctl &= ~PCI_EXP_DEVCTL_READRQ;
>> >
>> >
>> >
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
