| Message ID | 20190807130038.26878-1-sudeep.holla@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | firmware: arm_scmi: Use {get,put}_unaligned_le32 accessors | expand |
Hi Sudeep, On Wed, 2019-08-07 at 14:00 +0100, Sudeep Holla wrote: > Instead of type-casting the {tx,rx}.buf all over the place while > accessing them to read/write __le32 from/to the firmware, let's use > the nice existing {get,put}_unaligned_le32 accessors to hide all the > type cast ugliness. > > Suggested-by: Philipp Zabel <p.zabel@pengutronix.de> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/firmware/arm_scmi/base.c | 2 +- > drivers/firmware/arm_scmi/clock.c | 10 ++++------ > drivers/firmware/arm_scmi/common.h | 2 ++ > drivers/firmware/arm_scmi/perf.c | 8 ++++---- > drivers/firmware/arm_scmi/power.c | 6 +++--- > drivers/firmware/arm_scmi/reset.c | 2 +- > drivers/firmware/arm_scmi/sensors.c | 12 +++++------- > 7 files changed, 20 insertions(+), 22 deletions(-) > > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c > index 204390297f4b..f804e8af6521 100644 > --- a/drivers/firmware/arm_scmi/base.c > +++ b/drivers/firmware/arm_scmi/base.c [...] > @@ -204,14 +204,12 @@ scmi_clock_rate_get(const struct scmi_handle *handle, u32 clk_id, u64 *value) > if (ret) > return ret; > > - *(__le32 *)t->tx.buf = cpu_to_le32(clk_id); > + put_unaligned_le32(clk_id, t->tx.buf); > > ret = scmi_do_xfer(handle, t); > if (!ret) { > - __le32 *pval = t->rx.buf; > - > - *value = le32_to_cpu(*pval); > - *value |= (u64)le32_to_cpu(*(pval + 1)) << 32; > + *value = get_unaligned_le32(t->rx.buf); > + *value |= (u64)get_unaligned_le32(t->rx.buf + 1) << 32; Isn't t->rx.buf a void pointer? If I am not mistaken, you'd either have to keep the pval local variables, or cast to (__le32 *) before doing pointer arithmetic. regards Philipp
On Wed, Aug 07, 2019 at 03:36:11PM +0200, Philipp Zabel wrote: > Hi Sudeep, > > On Wed, 2019-08-07 at 14:00 +0100, Sudeep Holla wrote: > > Instead of type-casting the {tx,rx}.buf all over the place while > > accessing them to read/write __le32 from/to the firmware, let's use > > the nice existing {get,put}_unaligned_le32 accessors to hide all the > > type cast ugliness. > > > > Suggested-by: Philipp Zabel <p.zabel@pengutronix.de> > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > --- > > drivers/firmware/arm_scmi/base.c | 2 +- > > drivers/firmware/arm_scmi/clock.c | 10 ++++------ > > drivers/firmware/arm_scmi/common.h | 2 ++ > > drivers/firmware/arm_scmi/perf.c | 8 ++++---- > > drivers/firmware/arm_scmi/power.c | 6 +++--- > > drivers/firmware/arm_scmi/reset.c | 2 +- > > drivers/firmware/arm_scmi/sensors.c | 12 +++++------- > > 7 files changed, 20 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c > > index 204390297f4b..f804e8af6521 100644 > > --- a/drivers/firmware/arm_scmi/base.c > > +++ b/drivers/firmware/arm_scmi/base.c > [...] > > @@ -204,14 +204,12 @@ scmi_clock_rate_get(const struct scmi_handle *handle, u32 clk_id, u64 *value) > > if (ret) > > return ret; > > > > - *(__le32 *)t->tx.buf = cpu_to_le32(clk_id); > > + put_unaligned_le32(clk_id, t->tx.buf); > > > > ret = scmi_do_xfer(handle, t); > > if (!ret) { > > - __le32 *pval = t->rx.buf; > > - > > - *value = le32_to_cpu(*pval); > > - *value |= (u64)le32_to_cpu(*(pval + 1)) << 32; > > + *value = get_unaligned_le32(t->rx.buf); > > + *value |= (u64)get_unaligned_le32(t->rx.buf + 1) << 32; > > Isn't t->rx.buf a void pointer? If I am not mistaken, you'd either have > to keep the pval local variables, or cast to (__le32 *) before doing > pointer arithmetic. > Ah right, that's the reason I added it at the first place. I will fix that. -- Regards, Sudeep
On 07/08/2019 14:57, Sudeep Holla wrote: > On Wed, Aug 07, 2019 at 03:36:11PM +0200, Philipp Zabel wrote: >> Hi Sudeep, >> >> On Wed, 2019-08-07 at 14:00 +0100, Sudeep Holla wrote: >>> Instead of type-casting the {tx,rx}.buf all over the place while >>> accessing them to read/write __le32 from/to the firmware, let's use >>> the nice existing {get,put}_unaligned_le32 accessors to hide all the >>> type cast ugliness. >>> >>> Suggested-by: Philipp Zabel <p.zabel@pengutronix.de> >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >>> --- >>> drivers/firmware/arm_scmi/base.c | 2 +- >>> drivers/firmware/arm_scmi/clock.c | 10 ++++------ >>> drivers/firmware/arm_scmi/common.h | 2 ++ >>> drivers/firmware/arm_scmi/perf.c | 8 ++++---- >>> drivers/firmware/arm_scmi/power.c | 6 +++--- >>> drivers/firmware/arm_scmi/reset.c | 2 +- >>> drivers/firmware/arm_scmi/sensors.c | 12 +++++------- >>> 7 files changed, 20 insertions(+), 22 deletions(-) >>> >>> diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c >>> index 204390297f4b..f804e8af6521 100644 >>> --- a/drivers/firmware/arm_scmi/base.c >>> +++ b/drivers/firmware/arm_scmi/base.c >> [...] >>> @@ -204,14 +204,12 @@ scmi_clock_rate_get(const struct scmi_handle *handle, u32 clk_id, u64 *value) >>> if (ret) >>> return ret; >>> >>> - *(__le32 *)t->tx.buf = cpu_to_le32(clk_id); >>> + put_unaligned_le32(clk_id, t->tx.buf); >>> >>> ret = scmi_do_xfer(handle, t); >>> if (!ret) { >>> - __le32 *pval = t->rx.buf; >>> - >>> - *value = le32_to_cpu(*pval); >>> - *value |= (u64)le32_to_cpu(*(pval + 1)) << 32; >>> + *value = get_unaligned_le32(t->rx.buf); >>> + *value |= (u64)get_unaligned_le32(t->rx.buf + 1) << 32; >> >> Isn't t->rx.buf a void pointer? If I am not mistaken, you'd either have >> to keep the pval local variables, or cast to (__le32 *) before doing >> pointer arithmetic. >> > > Ah right, that's the reason I added it at the first place. I will fix that. Couldn't you just use get_unaligned_le64() here anyway? Robin.
On Wed, Aug 07, 2019 at 03:07:39PM +0100, Robin Murphy wrote: > On 07/08/2019 14:57, Sudeep Holla wrote: > > On Wed, Aug 07, 2019 at 03:36:11PM +0200, Philipp Zabel wrote: > > > Hi Sudeep, > > > > > > On Wed, 2019-08-07 at 14:00 +0100, Sudeep Holla wrote: > > > > Instead of type-casting the {tx,rx}.buf all over the place while > > > > accessing them to read/write __le32 from/to the firmware, let's use > > > > the nice existing {get,put}_unaligned_le32 accessors to hide all the > > > > type cast ugliness. > > > > > > > > Suggested-by: Philipp Zabel <p.zabel@pengutronix.de> > > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > > > --- > > > > drivers/firmware/arm_scmi/base.c | 2 +- > > > > drivers/firmware/arm_scmi/clock.c | 10 ++++------ > > > > drivers/firmware/arm_scmi/common.h | 2 ++ > > > > drivers/firmware/arm_scmi/perf.c | 8 ++++---- > > > > drivers/firmware/arm_scmi/power.c | 6 +++--- > > > > drivers/firmware/arm_scmi/reset.c | 2 +- > > > > drivers/firmware/arm_scmi/sensors.c | 12 +++++------- > > > > 7 files changed, 20 insertions(+), 22 deletions(-) > > > > > > > > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c > > > > index 204390297f4b..f804e8af6521 100644 > > > > --- a/drivers/firmware/arm_scmi/base.c > > > > +++ b/drivers/firmware/arm_scmi/base.c > > > [...] > > > > @@ -204,14 +204,12 @@ scmi_clock_rate_get(const struct scmi_handle *handle, u32 clk_id, u64 *value) > > > > if (ret) > > > > return ret; > > > > > > > > - *(__le32 *)t->tx.buf = cpu_to_le32(clk_id); > > > > + put_unaligned_le32(clk_id, t->tx.buf); > > > > > > > > ret = scmi_do_xfer(handle, t); > > > > if (!ret) { > > > > - __le32 *pval = t->rx.buf; > > > > - > > > > - *value = le32_to_cpu(*pval); > > > > - *value |= (u64)le32_to_cpu(*(pval + 1)) << 32; > > > > + *value = get_unaligned_le32(t->rx.buf); > > > > + *value |= (u64)get_unaligned_le32(t->rx.buf + 1) << 32; > > > > > > Isn't t->rx.buf a void pointer? If I am not mistaken, you'd either have > > > to keep the pval local variables, or cast to (__le32 *) before doing > > > pointer arithmetic. > > > > > > > Ah right, that's the reason I added it at the first place. I will fix that. > > Couldn't you just use get_unaligned_le64() here anyway? Indeed, that's what I found as I wanted to avoid pval, testing now. -- Regards, Sudeep
From: Sudeep Holla > Sent: 07 August 2019 14:01 > > Instead of type-casting the {tx,rx}.buf all over the place while > accessing them to read/write __le32 from/to the firmware, let's use > the nice existing {get,put}_unaligned_le32 accessors to hide all the > type cast ugliness. Why the 'unaligned' accessors? > - *(__le32 *)t->tx.buf = cpu_to_le32(id); > + put_unaligned_le32(id, t->tx.buf); These will be expensive if the cpu doesn't support them. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, Aug 07, 2019 at 03:18:59PM +0000, David Laight wrote: > From: Sudeep Holla > > Sent: 07 August 2019 14:01 > > > > Instead of type-casting the {tx,rx}.buf all over the place while > > accessing them to read/write __le32 from/to the firmware, let's use > > the nice existing {get,put}_unaligned_le32 accessors to hide all the > > type cast ugliness. > > Why the 'unaligned' accessors? > Since the firmware run in LE, we do byte-swapping anyways. > > - *(__le32 *)t->tx.buf = cpu_to_le32(id); > > + put_unaligned_le32(id, t->tx.buf); > If you look at the generic definition for put_unaligned_le32, it's exactly the same as what I am replacing with the call. So nothing changes IIUC. In fact, I see that all the helper in unaligned/access_ok.h just do the byte-swapping. > These will be expensive if the cpu doesn't support them. The SCMI is currently used only on ARM platforms which have HAVE_EFFICIENT_UNALIGNED_ACCESS defined. -- Regards, Sudeep
diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c index 204390297f4b..f804e8af6521 100644 --- a/drivers/firmware/arm_scmi/base.c +++ b/drivers/firmware/arm_scmi/base.c @@ -204,7 +204,7 @@ static int scmi_base_discover_agent_get(const struct scmi_handle *handle, if (ret) return ret; - *(__le32 *)t->tx.buf = cpu_to_le32(id); + put_unaligned_le32(id, t->tx.buf); ret = scmi_do_xfer(handle, t); if (!ret) diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c index 4a32ae1822a3..199a668ea885 100644 --- a/drivers/firmware/arm_scmi/clock.c +++ b/drivers/firmware/arm_scmi/clock.c @@ -107,7 +107,7 @@ static int scmi_clock_attributes_get(const struct scmi_handle *handle, if (ret) return ret; - *(__le32 *)t->tx.buf = cpu_to_le32(clk_id); + put_unaligned_le32(clk_id, t->tx.buf); attr = t->rx.buf; ret = scmi_do_xfer(handle, t); @@ -204,14 +204,12 @@ scmi_clock_rate_get(const struct scmi_handle *handle, u32 clk_id, u64 *value) if (ret) return ret; - *(__le32 *)t->tx.buf = cpu_to_le32(clk_id); + put_unaligned_le32(clk_id, t->tx.buf); ret = scmi_do_xfer(handle, t); if (!ret) { - __le32 *pval = t->rx.buf; - - *value = le32_to_cpu(*pval); - *value |= (u64)le32_to_cpu(*(pval + 1)) << 32; + *value = get_unaligned_le32(t->rx.buf); + *value |= (u64)get_unaligned_le32(t->rx.buf + 1) << 32; } scmi_xfer_put(handle, t); diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h index 43884e4ceac5..5237c2ff79fe 100644 --- a/drivers/firmware/arm_scmi/common.h +++ b/drivers/firmware/arm_scmi/common.h @@ -15,6 +15,8 @@ #include <linux/scmi_protocol.h> #include <linux/types.h> +#include <asm/unaligned.h> + #define PROTOCOL_REV_MINOR_MASK GENMASK(15, 0) #define PROTOCOL_REV_MAJOR_MASK GENMASK(31, 16) #define PROTOCOL_REV_MAJOR(x) (u16)(FIELD_GET(PROTOCOL_REV_MAJOR_MASK, (x))) diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c index fb7f6cab2c11..9b338e66a24e 100644 --- a/drivers/firmware/arm_scmi/perf.c +++ b/drivers/firmware/arm_scmi/perf.c @@ -195,7 +195,7 @@ scmi_perf_domain_attributes_get(const struct scmi_handle *handle, u32 domain, if (ret) return ret; - *(__le32 *)t->tx.buf = cpu_to_le32(domain); + put_unaligned_le32(domain, t->tx.buf); attr = t->rx.buf; ret = scmi_do_xfer(handle, t); @@ -380,7 +380,7 @@ static int scmi_perf_mb_limits_get(const struct scmi_handle *handle, u32 domain, if (ret) return ret; - *(__le32 *)t->tx.buf = cpu_to_le32(domain); + put_unaligned_le32(domain, t->tx.buf); ret = scmi_do_xfer(handle, t); if (!ret) { @@ -459,11 +459,11 @@ static int scmi_perf_mb_level_get(const struct scmi_handle *handle, u32 domain, return ret; t->hdr.poll_completion = poll; - *(__le32 *)t->tx.buf = cpu_to_le32(domain); + put_unaligned_le32(domain, t->tx.buf); ret = scmi_do_xfer(handle, t); if (!ret) - *level = le32_to_cpu(*(__le32 *)t->rx.buf); + *level = get_unaligned_le32(t->rx.buf); scmi_xfer_put(handle, t); return ret; diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c index 62f3401a1f01..5abef7079c0a 100644 --- a/drivers/firmware/arm_scmi/power.c +++ b/drivers/firmware/arm_scmi/power.c @@ -96,7 +96,7 @@ scmi_power_domain_attributes_get(const struct scmi_handle *handle, u32 domain, if (ret) return ret; - *(__le32 *)t->tx.buf = cpu_to_le32(domain); + put_unaligned_le32(domain, t->tx.buf); attr = t->rx.buf; ret = scmi_do_xfer(handle, t); @@ -147,11 +147,11 @@ scmi_power_state_get(const struct scmi_handle *handle, u32 domain, u32 *state) if (ret) return ret; - *(__le32 *)t->tx.buf = cpu_to_le32(domain); + put_unaligned_le32(domain, t->tx.buf); ret = scmi_do_xfer(handle, t); if (!ret) - *state = le32_to_cpu(*(__le32 *)t->rx.buf); + *state = get_unaligned_le32(t->rx.buf); scmi_xfer_put(handle, t); return ret; diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c index 11cb8b5ccf34..c1d67a2af12f 100644 --- a/drivers/firmware/arm_scmi/reset.c +++ b/drivers/firmware/arm_scmi/reset.c @@ -88,7 +88,7 @@ scmi_reset_domain_attributes_get(const struct scmi_handle *handle, u32 domain, if (ret) return ret; - *(__le32 *)t->tx.buf = cpu_to_le32(domain); + put_unaligned_le32(domain, t->tx.buf); attr = t->rx.buf; ret = scmi_do_xfer(handle, t); diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c index 7570308a16a0..5b330619a025 100644 --- a/drivers/firmware/arm_scmi/sensors.c +++ b/drivers/firmware/arm_scmi/sensors.c @@ -120,7 +120,7 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle, do { /* Set the number of sensors to be skipped/already read */ - *(__le32 *)t->tx.buf = cpu_to_le32(desc_index); + put_unaligned_le32(desc_index, t->tx.buf); ret = scmi_do_xfer(handle, t); if (ret) @@ -217,7 +217,6 @@ static int scmi_sensor_reading_get(const struct scmi_handle *handle, u32 sensor_id, u64 *value) { int ret; - __le32 *pval; struct scmi_xfer *t; struct scmi_msg_sensor_reading_get *sensor; struct sensors_info *si = handle->sensor_priv; @@ -229,7 +228,6 @@ static int scmi_sensor_reading_get(const struct scmi_handle *handle, if (ret) return ret; - pval = t->rx.buf; sensor = t->tx.buf; sensor->id = cpu_to_le32(sensor_id); @@ -237,15 +235,15 @@ static int scmi_sensor_reading_get(const struct scmi_handle *handle, sensor->flags = cpu_to_le32(SENSOR_READ_ASYNC); ret = scmi_do_xfer_with_response(handle, t); if (!ret) { - *value = le32_to_cpu(*(pval + 1)); - *value |= (u64)le32_to_cpu(*(pval + 2)) << 32; + *value = get_unaligned_le32(t->rx.buf + 1); + *value |= (u64)get_unaligned_le32(t->rx.buf + 2) << 32; } } else { sensor->flags = cpu_to_le32(0); ret = scmi_do_xfer(handle, t); if (!ret) { - *value = le32_to_cpu(*pval); - *value |= (u64)le32_to_cpu(*(pval + 1)) << 32; + *value = get_unaligned_le32(t->rx.buf); + *value |= (u64)get_unaligned_le32(t->rx.buf + 1) << 32; } }
Instead of type-casting the {tx,rx}.buf all over the place while accessing them to read/write __le32 from/to the firmware, let's use the nice existing {get,put}_unaligned_le32 accessors to hide all the type cast ugliness. Suggested-by: Philipp Zabel <p.zabel@pengutronix.de> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/firmware/arm_scmi/base.c | 2 +- drivers/firmware/arm_scmi/clock.c | 10 ++++------ drivers/firmware/arm_scmi/common.h | 2 ++ drivers/firmware/arm_scmi/perf.c | 8 ++++---- drivers/firmware/arm_scmi/power.c | 6 +++--- drivers/firmware/arm_scmi/reset.c | 2 +- drivers/firmware/arm_scmi/sensors.c | 12 +++++------- 7 files changed, 20 insertions(+), 22 deletions(-)