| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20210527192657.GA3454023@roeck-us.net>
Date: Thu, 27 May 2021 12:26:57 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Robert Marko <robert.marko@...tura.hr>
Cc: jdelvare@...e.com, corbet@....net, linux-hwmon@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Luka Perkov <luka.perkov@...tura.hr>, jmp@...phyte.org,
Paul Menzel <pmenzel@...gen.mpg.de>,
Donald Buczek <buczek@...gen.mpg.de>
Subject: Re: [PATCH 1/3] hwmon: (pmbus) Add driver for Delta DPS-920AB PSU
On Thu, May 27, 2021 at 09:14:03PM +0200, Robert Marko wrote:
> On Thu, May 27, 2021 at 6:37 PM Guenter Roeck <linux@...ck-us.net> wrote:
> >
> > On 5/27/21 7:58 AM, Robert Marko wrote:
> > [ ... ]
> >
> > >>>>>> I tried applying the block support for mv64xx as well:
> > >>>>>> https://patchwork.ozlabs.org/project/linux-i2c/patch/20200118115820.9080-1-fuga@studiofuga.com/
> > >>>>
> > >>>> That patch would be needed, but it looks buggy to me. This chunk:
> > >>>>
> > >>>> + drv_data->effective_length = data+1;
> > >>>> + drv_data->bytes_left = data+1;
> > >>>> + drv_data->msg->len = data+1;
> > >>>>
> > >>>> should be:
> > >>>>
> > >>>> + drv_data->effective_length = data+1;
> > >>>> + drv_data->bytes_left = data;
> > >>>> + drv_data->msg->len = data+1;
> > >>>>
> > >>>> It should also make sure that 'data' is not larger than I2C_SMBUS_BLOCK_MAX,
> > >>>> and bail out if it isn't.
> > >>>
> > >>> Yeah, I did not check the contents, I just saw 2 reviews and tested it
> > >>> since it can't hurt.
> > >>
> > >> That patch doesn't work at all. Make the above change, and also change
> > >> the type of effective_length from u32 to int, and try again.
> > >
> > > I was just looking and it, and doing the changes you recommended make
> > > no difference at all.
> > >
> >
> > Is the i2c controller compatible with marvell,mv78230-i2c ?
> > The block transfers would not work in that case. Let me know
> > and I'll send you a patch that might fix it.
>
> Yes, the CPU is Armada 7040 and I can see that compatible is
> marvell,mv78230-i2c indeed.
>
> It would be great if you have the patch,
See below.
Guenter
---
>From 41f6113560e3082293cef6e8f2013e70ee8f54db Mon Sep 17 00:00:00 2001
From: Federico Fuga <fuga@...diofuga.com>
Date: Sat, 18 Jan 2020 12:58:20 +0100
Subject: [PATCH] i2c: mv64xxx: Implement I2C_M_RECV_LEN and
I2C_FUNC_SMBUS_READ_BLOCK_DATA
The i2c_mv64xxx driver doesn't implement the I2C_M_REC_LEN function
essential to allow blocks with variable length to be read from an i2c
slave.
This is needed to implement the SMBus Read Block Data function.
This patch implements the function by changing the bytes_left and
msg len on the fly if the flag is specified.
It has been successfully tested on Allwinner A33 with a special
i2c chip that returns variable length blocks on reading.
Signed-off-by: Federico Fuga <fuga@...diofuga.com>
Acked-by: Gregory CLEMENT <gregory.clement@...tlin.com>
Link: https://lore.kernel.org/r/20200118115820.9080-1-fuga@studiofuga.com
[groeck: Various fixes]
Signed-off-by: Guenter Roeck <linux@...ck-us.net>
---
drivers/i2c/busses/i2c-mv64xxx.c | 75 ++++++++++++++++++++++++++------
1 file changed, 61 insertions(+), 14 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 5c8e94b6cdb5..9dda90359966 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -341,7 +341,17 @@ static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
{
drv_data->msg = drv_data->msgs;
drv_data->byte_posn = 0;
- drv_data->bytes_left = drv_data->msg->len;
+
+ /*
+ * If we should retrieve the length from the buffer, make sure
+ * to read enough bytes to avoid sending the STOP bit after
+ * the read if the first byte
+ */
+ if (drv_data->msg->flags & I2C_M_RECV_LEN)
+ drv_data->bytes_left = 3;
+ else
+ drv_data->bytes_left = drv_data->msg->len;
+
drv_data->aborting = 0;
drv_data->rc = 0;
@@ -350,6 +360,47 @@ static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
drv_data->reg_base + drv_data->reg_offsets.control);
}
+static void
+mv64xxx_i2c_do_send_stop(struct mv64xxx_i2c_data *drv_data)
+{
+ drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
+ writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
+ drv_data->reg_base + drv_data->reg_offsets.control);
+ drv_data->block = 0;
+ if (drv_data->errata_delay)
+ udelay(5);
+
+ wake_up(&drv_data->waitq);
+}
+
+static void
+mv64xxx_i2c_do_read_data(struct mv64xxx_i2c_data *drv_data)
+{
+ u8 data;
+
+ data = readl(drv_data->reg_base + drv_data->reg_offsets.data);
+ drv_data->msg->buf[drv_data->byte_posn++] = data;
+
+ if (drv_data->msg->flags & I2C_M_RECV_LEN) {
+ if (!data || data > I2C_SMBUS_BLOCK_MAX) {
+ /*
+ * FIXME
+ * Abort and report error. Needs to be
+ * verified/tested on real hardware.
+ */
+ drv_data->rc = -EPROTO;
+ mv64xxx_i2c_do_send_stop(drv_data);
+ return;
+ }
+ drv_data->msg->flags &= ~I2C_M_RECV_LEN;
+ drv_data->bytes_left = data;
+ drv_data->msg->len = data + 1;
+ }
+
+ writel(drv_data->cntl_bits,
+ drv_data->reg_base + drv_data->reg_offsets.control);
+}
+
static void
mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
{
@@ -400,23 +451,13 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
break;
case MV64XXX_I2C_ACTION_RCV_DATA:
- drv_data->msg->buf[drv_data->byte_posn++] =
- readl(drv_data->reg_base + drv_data->reg_offsets.data);
- writel(drv_data->cntl_bits,
- drv_data->reg_base + drv_data->reg_offsets.control);
+ mv64xxx_i2c_do_read_data(drv_data);
break;
case MV64XXX_I2C_ACTION_RCV_DATA_STOP:
drv_data->msg->buf[drv_data->byte_posn++] =
readl(drv_data->reg_base + drv_data->reg_offsets.data);
- drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
- writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
- drv_data->reg_base + drv_data->reg_offsets.control);
- drv_data->block = 0;
- if (drv_data->errata_delay)
- udelay(5);
-
- wake_up(&drv_data->waitq);
+ mv64xxx_i2c_do_send_stop(drv_data);
break;
case MV64XXX_I2C_ACTION_INVALID:
@@ -680,6 +721,10 @@ mv64xxx_i2c_can_offload(struct mv64xxx_i2c_data *drv_data)
if (!drv_data->offload_enabled)
return false;
+ /* Offload not supported for block data transfers */
+ if (msgs[0].flags & I2C_M_RECV_LEN)
+ return false;
+
/*
* We can offload a transaction consisting of a single
* message, as long as the message has a length between 1 and
@@ -697,6 +742,7 @@ mv64xxx_i2c_can_offload(struct mv64xxx_i2c_data *drv_data)
mv64xxx_i2c_valid_offload_sz(msgs) &&
mv64xxx_i2c_valid_offload_sz(msgs + 1) &&
!(msgs[0].flags & I2C_M_RD) &&
+ !(msgs[1].flags & I2C_M_RECV_LEN) &&
msgs[1].flags & I2C_M_RD)
return true;
@@ -713,7 +759,8 @@ mv64xxx_i2c_can_offload(struct mv64xxx_i2c_data *drv_data)
static u32
mv64xxx_i2c_functionality(struct i2c_adapter *adap)
{
- return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL;
+ return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR |
+ I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_EMUL;
}
static int
--
2.25.1
Powered by blists - more mailing lists