* [PATCH v2] PCI/VPD: Use unaligned access helpers in pci_vpd_read
@ 2021-05-07 22:29 Heiner Kallweit
2021-05-10 6:36 ` Christoph Hellwig
0 siblings, 1 reply; 6+ messages in thread
From: Heiner Kallweit @ 2021-05-07 22:29 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, Bjorn Helgaas
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(-)
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);
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] PCI/VPD: Use unaligned access helpers in pci_vpd_read
2021-05-07 22:29 [PATCH v2] PCI/VPD: Use unaligned access helpers in pci_vpd_read Heiner Kallweit
@ 2021-05-10 6:36 ` Christoph Hellwig
2021-05-10 7:09 ` Heiner Kallweit
0 siblings, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2021-05-10 6:36 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Bjorn Helgaas, linux-pci, Bjorn Helgaas
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;
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] PCI/VPD: Use unaligned access helpers in pci_vpd_read
2021-05-10 6:36 ` Christoph Hellwig
@ 2021-05-10 7:09 ` Heiner Kallweit
2021-05-12 7:21 ` Christoph Hellwig
0 siblings, 1 reply; 6+ messages in thread
From: Heiner Kallweit @ 2021-05-10 7:09 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Bjorn Helgaas, linux-pci, Bjorn Helgaas
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] PCI/VPD: Use unaligned access helpers in pci_vpd_read
2021-05-10 7:09 ` Heiner Kallweit
@ 2021-05-12 7:21 ` Christoph Hellwig
2021-05-12 7:56 ` Heiner Kallweit
0 siblings, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2021-05-12 7:21 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Christoph Hellwig, Bjorn Helgaas, linux-pci, Bjorn Helgaas
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] PCI/VPD: Use unaligned access helpers in pci_vpd_read
2021-05-12 7:21 ` Christoph Hellwig
@ 2021-05-12 7:56 ` Heiner Kallweit
2021-05-12 17:35 ` Christoph Hellwig
0 siblings, 1 reply; 6+ messages in thread
From: Heiner Kallweit @ 2021-05-12 7:56 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Bjorn Helgaas, linux-pci, Bjorn Helgaas
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] PCI/VPD: Use unaligned access helpers in pci_vpd_read
2021-05-12 7:56 ` Heiner Kallweit
@ 2021-05-12 17:35 ` Christoph Hellwig
0 siblings, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2021-05-12 17:35 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Christoph Hellwig, Bjorn Helgaas, linux-pci, Bjorn Helgaas
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-12 18:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-07 22:29 [PATCH v2] PCI/VPD: Use unaligned access helpers in pci_vpd_read Heiner Kallweit
2021-05-10 6:36 ` Christoph Hellwig
2021-05-10 7:09 ` Heiner Kallweit
2021-05-12 7:21 ` Christoph Hellwig
2021-05-12 7:56 ` Heiner Kallweit
2021-05-12 17:35 ` Christoph Hellwig
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.