| Message ID | 79d337f4-25d6-9024-2cbd-63801cbd9992@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI/VPD: Use unaligned access helper in pci_vpd_write | expand |
On 18.04.2021 11:19, Heiner Kallweit wrote: > Use helper get_unaligned_le32() to simplify the code. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/pci/vpd.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c > index 60573f27a..2888bb300 100644 > --- a/drivers/pci/vpd.c > +++ b/drivers/pci/vpd.c > @@ -9,6 +9,7 @@ > #include <linux/delay.h> > #include <linux/export.h> > #include <linux/sched/signal.h> > +#include <asm/unaligned.h> > #include "pci.h" > > /* VPD access through PCI 2.2+ VPD capability */ > @@ -235,10 +236,9 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, > } > > static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, > - const void *arg) > + const void *buf) > { > struct pci_vpd *vpd = dev->vpd; > - const u8 *buf = arg; > loff_t end = pos + count; > int ret = 0; > > @@ -264,14 +264,8 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, > goto out; > > while (pos < end) { > - u32 val; > - > - val = *buf++; > - val |= *buf++ << 8; > - val |= *buf++ << 16; > - val |= *buf++ << 24; > - > - ret = pci_user_write_config_dword(dev, vpd->cap + PCI_VPD_DATA, val); > + ret = pci_user_write_config_dword(dev, vpd->cap + PCI_VPD_DATA, > + get_unaligned_le32(buf)); > if (ret < 0) > break; > ret = pci_user_write_config_word(dev, vpd->cap + PCI_VPD_ADDR, > @@ -286,6 +280,7 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, > break; > > pos += sizeof(u32); > + buf += sizeof(u32); > } > out: > mutex_unlock(&vpd->lock); > Please disregard this patch. According to PCI 2.2 spec the address needs to be dword-aligned. I will address this in a separate patch.
On 19.04.2021 13:10, Heiner Kallweit wrote: > On 18.04.2021 11:19, Heiner Kallweit wrote: >> Use helper get_unaligned_le32() to simplify the code. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/pci/vpd.c | 15 +++++---------- >> 1 file changed, 5 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c >> index 60573f27a..2888bb300 100644 >> --- a/drivers/pci/vpd.c >> +++ b/drivers/pci/vpd.c >> @@ -9,6 +9,7 @@ >> #include <linux/delay.h> >> #include <linux/export.h> >> #include <linux/sched/signal.h> >> +#include <asm/unaligned.h> >> #include "pci.h" >> >> /* VPD access through PCI 2.2+ VPD capability */ >> @@ -235,10 +236,9 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, >> } >> >> static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, >> - const void *arg) >> + const void *buf) >> { >> struct pci_vpd *vpd = dev->vpd; >> - const u8 *buf = arg; >> loff_t end = pos + count; >> int ret = 0; >> >> @@ -264,14 +264,8 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, >> goto out; >> >> while (pos < end) { >> - u32 val; >> - >> - val = *buf++; >> - val |= *buf++ << 8; >> - val |= *buf++ << 16; >> - val |= *buf++ << 24; >> - >> - ret = pci_user_write_config_dword(dev, vpd->cap + PCI_VPD_DATA, val); >> + ret = pci_user_write_config_dword(dev, vpd->cap + PCI_VPD_DATA, >> + get_unaligned_le32(buf)); >> if (ret < 0) >> break; >> ret = pci_user_write_config_word(dev, vpd->cap + PCI_VPD_ADDR, >> @@ -286,6 +280,7 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, >> break; >> >> pos += sizeof(u32); >> + buf += sizeof(u32); >> } >> out: >> mutex_unlock(&vpd->lock); >> > > Please disregard this patch. According to PCI 2.2 spec the address needs to be dword-aligned. > I will address this in a separate patch. > Sorry, I just confused something and wrote before finished thinking. The change is correct.
On Sun, Apr 18, 2021 at 11:19:29AM +0200, Heiner Kallweit wrote: > Use helper get_unaligned_le32() to simplify the code. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/pci/vpd.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c > index 60573f27a..2888bb300 100644 > --- a/drivers/pci/vpd.c > +++ b/drivers/pci/vpd.c > @@ -9,6 +9,7 @@ > #include <linux/delay.h> > #include <linux/export.h> > #include <linux/sched/signal.h> > +#include <asm/unaligned.h> > #include "pci.h" > > /* VPD access through PCI 2.2+ VPD capability */ > @@ -235,10 +236,9 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, > } > > static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, > - const void *arg) > + const void *buf) > { > struct pci_vpd *vpd = dev->vpd; > - const u8 *buf = arg; > loff_t end = pos + count; > int ret = 0; > > @@ -264,14 +264,8 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, > goto out; > > while (pos < end) { > - u32 val; > - > - val = *buf++; > - val |= *buf++ << 8; > - val |= *buf++ << 16; > - val |= *buf++ << 24; > - > - ret = pci_user_write_config_dword(dev, vpd->cap + PCI_VPD_DATA, val); > + ret = pci_user_write_config_dword(dev, vpd->cap + PCI_VPD_DATA, > + get_unaligned_le32(buf)); If I understand correctly, get_unaligned_le32() is equivalent to the open-coded "val = *buf++; ..." In other words, this doesn't fix a bug on any platforms, and it doesn't change the bits written to VPD. Right? If so, this makes sense. I'd like it better if we could also make pci_vpd_read() look more symmetric, since it has similar byte order-related code. The read side is a little more complicated since the size need not be 4-byte aligned. But maybe there's a way? Seems like there should be other examples of this sort of thing, but my quick look didn't find one. > if (ret < 0) > break; > ret = pci_user_write_config_word(dev, vpd->cap + PCI_VPD_ADDR, > @@ -286,6 +280,7 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, > break; > > pos += sizeof(u32); > + buf += sizeof(u32); > } > out: > mutex_unlock(&vpd->lock); > -- > 2.31.1 >
On 30.04.2021 22:29, Bjorn Helgaas wrote: > On Sun, Apr 18, 2021 at 11:19:29AM +0200, Heiner Kallweit wrote: >> Use helper get_unaligned_le32() to simplify the code. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/pci/vpd.c | 15 +++++---------- >> 1 file changed, 5 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c >> index 60573f27a..2888bb300 100644 >> --- a/drivers/pci/vpd.c >> +++ b/drivers/pci/vpd.c >> @@ -9,6 +9,7 @@ >> #include <linux/delay.h> >> #include <linux/export.h> >> #include <linux/sched/signal.h> >> +#include <asm/unaligned.h> >> #include "pci.h" >> >> /* VPD access through PCI 2.2+ VPD capability */ >> @@ -235,10 +236,9 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, >> } >> >> static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, >> - const void *arg) >> + const void *buf) >> { >> struct pci_vpd *vpd = dev->vpd; >> - const u8 *buf = arg; >> loff_t end = pos + count; >> int ret = 0; >> >> @@ -264,14 +264,8 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, >> goto out; >> >> while (pos < end) { >> - u32 val; >> - >> - val = *buf++; >> - val |= *buf++ << 8; >> - val |= *buf++ << 16; >> - val |= *buf++ << 24; >> - >> - ret = pci_user_write_config_dword(dev, vpd->cap + PCI_VPD_DATA, val); >> + ret = pci_user_write_config_dword(dev, vpd->cap + PCI_VPD_DATA, >> + get_unaligned_le32(buf)); > > If I understand correctly, get_unaligned_le32() is equivalent to the > open-coded "val = *buf++; ..." In other words, this doesn't fix a bug > on any platforms, and it doesn't change the bits written to VPD. > Right? > Right. > If so, this makes sense. I'd like it better if we could also make > pci_vpd_read() look more symmetric, since it has similar byte > order-related code. The read side is a little more complicated since > the size need not be 4-byte aligned. But maybe there's a way? Seems > like there should be other examples of this sort of thing, but my > quick look didn't find one. > I also find the read side a little bit hard to understand. Let me check whether I find some way to improve the code. >> if (ret < 0) >> break; >> ret = pci_user_write_config_word(dev, vpd->cap + PCI_VPD_ADDR, >> @@ -286,6 +280,7 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, >> break; >> >> pos += sizeof(u32); >> + buf += sizeof(u32); >> } >> out: >> mutex_unlock(&vpd->lock); >> -- >> 2.31.1 >>
diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c index 60573f27a..2888bb300 100644 --- a/drivers/pci/vpd.c +++ b/drivers/pci/vpd.c @@ -9,6 +9,7 @@ #include <linux/delay.h> #include <linux/export.h> #include <linux/sched/signal.h> +#include <asm/unaligned.h> #include "pci.h" /* VPD access through PCI 2.2+ VPD capability */ @@ -235,10 +236,9 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count, } static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, - const void *arg) + const void *buf) { struct pci_vpd *vpd = dev->vpd; - const u8 *buf = arg; loff_t end = pos + count; int ret = 0; @@ -264,14 +264,8 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, goto out; while (pos < end) { - u32 val; - - val = *buf++; - val |= *buf++ << 8; - val |= *buf++ << 16; - val |= *buf++ << 24; - - ret = pci_user_write_config_dword(dev, vpd->cap + PCI_VPD_DATA, val); + ret = pci_user_write_config_dword(dev, vpd->cap + PCI_VPD_DATA, + get_unaligned_le32(buf)); if (ret < 0) break; ret = pci_user_write_config_word(dev, vpd->cap + PCI_VPD_ADDR, @@ -286,6 +280,7 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count, break; pos += sizeof(u32); + buf += sizeof(u32); } out: mutex_unlock(&vpd->lock);
Use helper get_unaligned_le32() to simplify the code. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/pci/vpd.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)