Re: [PATCH 6/7] pciehp: Add Disable/enable link functions

[Yinghai Lu]

On Mon, Jan 23, 2012 at 8:13 AM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Sat, Jan 21, 2012 at 1:52 AM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
>> +
>> +       retval = pciehp_readw(ctrl, PCI_EXP_LNKCTL, &lnk_ctrl);
>> +       if (retval) {
>> +               ctrl_err(ctrl, "Cannot read LNKCTRL register\n");
>> +               return retval;
>> +       }
>
> Is there really any point at all in checking the return value of that
> readw() function?
>
> Nobody actually ever sets it. We should stop even bothering to pretend
> to care. There is no return value in real life.
>
> If the device doesn't exist or the read fails, you will never ever get
> an error *anyway*. You'll likely get 0xffff, and possibly a machine
> check exception. And that is unlikely to ever change - even if the
> hardware were to do it, it's insane to do error checking on an
> individual IO level. It causes more problems than it could possibly
> ever solve - error checking has to be done at a different level than
> on some random individual access.
>
> We should make the return type of pci_read_config_word() be 'void',
> instead of having code like this that looks like it is careful, but in
> actual fact in reality is just totally pointless.

looks like

#define PCI_OP_READ(size,type,len) \
int pci_bus_read_config_##size \
        (struct pci_bus *bus, unsigned int devfn, int pos, type *value) \
{                                                                       \
        int res;                                                        \
        unsigned long flags;                                            \
        u32 data = 0;                                                   \
        if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;       \
        raw_spin_lock_irqsave(&pci_lock, flags);                        \
        res = bus->ops->read(bus, devfn, pos, len, &data);              \
        *value = (type)data;                                            \
        raw_spin_unlock_irqrestore(&pci_lock, flags);           \
        return res;                                                     \
}

will check register number. like we can not pass 0x01 but want to read
word and dword.

also bus ops read/write like pci_mmcfg_read/write will double check if
passed bus/dev/fn/reg
ranges.

Thanks

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/