| 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
| ||
|
Date: Thu, 27 May 2021 21:43:33 +0200
From: Robert Marko <robert.marko@...tura.hr>
To: Guenter Roeck <linux@...ck-us.net>
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 9:27 PM Guenter Roeck <linux@...ck-us.net> wrote:
>
> 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.
I can confirm that this does indeed enable block reads and
i2c_smbus_read_block_data()
works now.
PMBUS_MFR_ID and PMBUS_MFR_MODEL are indeed populated.
Regards,
Robert
>
> 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
>
--
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@...tura.hr
Web: www.sartura.hr
Powered by blists - more mailing lists