| Message ID | 5719b91c-9f91-0029-0a28-386f1cb29d31@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v2] PCI/VPD: Use unaligned access helpers in pci_vpd_read | expand |
On Sat, May 08, 2021 at 12:29:15AM +0200, Heiner Kallweit wrote: > + > + if (len == 4) { > + put_unaligned_le32(val, buf); > + } else { > + cpu_to_le32s(&val); > + memcpy(buf, (u8 *)&val + skip, len); cpu_to_le32s is a horrible API that breaks endianess annotations. Is the intent of this code to only put 16 bits in? Why not something like: switch (len) { case 4: put_unaligned_le32(val, buf); break; case 2: put_unaligned_le16(val, buf + 2); break; case 1: buf[3] = val; break; }
On 10.05.2021 08:36, Christoph Hellwig wrote: > On Sat, May 08, 2021 at 12:29:15AM +0200, Heiner Kallweit wrote: >> + >> + if (len == 4) { >> + put_unaligned_le32(val, buf); >> + } else { >> + cpu_to_le32s(&val); >> + memcpy(buf, (u8 *)&val + skip, len); > > cpu_to_le32s is a horrible API that breaks endianess annotations. > The endian-adjusted value is used just in the next line, so I think there's no risk of wrong usage. But yes, this function converts to le32 w/o using __bitwise. Therefore I understand the concern. > Is the intent of this code to only put 16 bits in? Why not something > like: > > switch (len) { > case 4: > put_unaligned_le32(val, buf); > break; > case 2: > put_unaligned_le16(val, buf + 2); > break; > case 1: > buf[3] = val; > break; > } > len can have any value 1 .. 4. Also the proposal doesn't consider the skip value.
On Mon, May 10, 2021 at 09:09:02AM +0200, Heiner Kallweit wrote: > len can have any value 1 .. 4. Also the proposal doesn't consider > the skip value. So what about just keeping the code as-is then? The existing version is much easier to read than the new one, has less branching and doesn't use an obscure API thast should not generally be used in driver code.
On 12.05.2021 09:21, Christoph Hellwig wrote: > On Mon, May 10, 2021 at 09:09:02AM +0200, Heiner Kallweit wrote: >> len can have any value 1 .. 4. Also the proposal doesn't consider >> the skip value. > > So what about just keeping the code as-is then? The existing version > is much easier to read than the new one, has less branching and doesn't > use an obscure API thast should not generally be used in driver code. > Which version is easier to read may be a question of personal taste. Using the unaligned access helper in pci_vpd_read() was asked for by Bjorn to complement a use of get_unaligned_le32() in pci_vpd_write(). So I leave it up to him. Functionally it's the same anyway.
On Wed, May 12, 2021 at 09:56:31AM +0200, Heiner Kallweit wrote: > Which version is easier to read may be a question of personal taste. > Using the unaligned access helper in pci_vpd_read() was asked for by > Bjorn to complement a use of get_unaligned_le32() in pci_vpd_write(). > So I leave it up to him. Functionally it's the same anyway. get_unaligned_le32 and friends are really nice helpers, but only when they fit the use case. In this case the existing code is a fairly easy to follow loop, while the "new" version is a mess by all counts.
diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c index 741e7a505..ab886e5f4 100644 --- a/drivers/pci/vpd.c +++ b/drivers/pci/vpd.c @@ -163,12 +163,11 @@ static int pci_vpd_wait(struct pci_dev *dev) } static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, - void *arg) + void *buf) { struct pci_vpd *vpd = dev->vpd; int ret; loff_t end = pos + count; - u8 *buf = arg; if (pos < 0) return -EINVAL; @@ -197,8 +196,8 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, goto out; while (pos < end) { + unsigned int len, skip; u32 val; - unsigned int i, skip; ret = pci_user_write_config_word(dev, vpd->cap + PCI_VPD_ADDR, pos & ~3); @@ -215,14 +214,17 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, break; skip = pos & 3; - for (i = 0; i < sizeof(u32); i++) { - if (i >= skip) { - *buf++ = val; - if (++pos == end) - break; - } - val >>= 8; + len = min_t(unsigned int, 4 - skip, end - pos); + + if (len == 4) { + put_unaligned_le32(val, buf); + } else { + cpu_to_le32s(&val); + memcpy(buf, (u8 *)&val + skip, len); } + + buf += len; + pos += len; } out: mutex_unlock(&vpd->lock);
With this patch we avoid some unnecessary looping if the platform defines HAVE_EFFICIENT_UNALIGNED_ACCESS and we make the code better readable. No functional change intended. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- v2: remove not needed tmpbuf variable --- drivers/pci/vpd.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)