[2/2] i2c: Add FORCE option

Message ID 20180613064054.20625-3-joel@jms.id.au
State Changes Requested
Headers show
Series
  • Restore OpenBMC P8 support
Related show

Commit Message

Joel Stanley June 13, 2018, 6:40 a.m.
On P8 machines with OpenBMC, the i2c address is claimed by the OCC hwmon
driver. We can ignore it and talk to the device anyway by passing
I2C_SLAVE_FORCE instead of I2C_SLAVE to the open ioctl.

This might not be the best idea, but it works fine for eg. getscom.

Signed-off-by: Joel Stanley <joel@jms.id.au>
--
We could put this behind a --force option.

Also, when the user does not pass --force, we could advise them to
unbind the hwmon driver. I don't know what OpenBMC userspace does when
you unbind it - it may decide to shut down the machine.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 libpdbg/i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alistair Popple June 15, 2018, 4:45 a.m. | #1
I've already taken the first patch in this series but I'm not sure about this
one, comments below.

On Wednesday, 13 June 2018 4:10:54 PM AEST Joel Stanley wrote:
> On P8 machines with OpenBMC, the i2c address is claimed by the OCC hwmon
> driver. We can ignore it and talk to the device anyway by passing
> I2C_SLAVE_FORCE instead of I2C_SLAVE to the open ioctl.
> 
> This might not be the best idea, but it works fine for eg. getscom.

Probably not :-)

At least I'd like to see some kind of warning to the user that we might be
breaking their I2C/OCC. Perhaps something along the lines of attempting a normal
I2C_SLAVE ioctl and if that fails print a warning that you should unbind the
hwmon driver before continuing with I2C_SLAVE_FORCE anyway. Or a --force flag but I'd
be happy enough with just a warning prior to breaking things.

- Alistair

> Signed-off-by: Joel Stanley <joel@jms.id.au>
> --
> We could put this behind a --force option.
> 
> Also, when the user does not pass --force, we could advise them to
> unbind the hwmon driver. I don't know what OpenBMC userspace does when
> you unbind it - it may decide to shut down the machine.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  libpdbg/i2c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libpdbg/i2c.c b/libpdbg/i2c.c
> index b1580e176023..aa0a73ef8c19 100644
> --- a/libpdbg/i2c.c
> +++ b/libpdbg/i2c.c
> @@ -34,7 +34,7 @@ struct i2c_data {
>  
>  static int i2c_set_addr(int fd, int addr)
>  {
> -	if (ioctl(fd, I2C_SLAVE, addr) < 0) {
> +	if (ioctl(fd, I2C_SLAVE_FORCE, addr) < 0) {
>  		PR_ERROR("Unable to set i2c slave address\n");
>  		return -1;
>  	}
>
Joel Stanley June 15, 2018, 4:47 a.m. | #2
On 15 June 2018 at 14:15, Alistair Popple <alistair@popple.id.au> wrote:
> I've already taken the first patch in this series but I'm not sure about this
> one, comments below.
>
> On Wednesday, 13 June 2018 4:10:54 PM AEST Joel Stanley wrote:
>> On P8 machines with OpenBMC, the i2c address is claimed by the OCC hwmon
>> driver. We can ignore it and talk to the device anyway by passing
>> I2C_SLAVE_FORCE instead of I2C_SLAVE to the open ioctl.
>>
>> This might not be the best idea, but it works fine for eg. getscom.
>
> Probably not :-)
>
> At least I'd like to see some kind of warning to the user that we might be
> breaking their I2C/OCC. Perhaps something along the lines of attempting a normal
> I2C_SLAVE ioctl and if that fails print a warning that you should unbind the
> hwmon driver before continuing with I2C_SLAVE_FORCE anyway. Or a --force flag but I'd
> be happy enough with just a warning prior to breaking things.

As we found it testing, this won't break their OCC as much as the OCC
will break their pdbgging.

We could add a --force, as this appears to work okay short command -
getscom and friends.

I will send a v2 with your suggestion.

Cheers,

Joel

Patch

diff --git a/libpdbg/i2c.c b/libpdbg/i2c.c
index b1580e176023..aa0a73ef8c19 100644
--- a/libpdbg/i2c.c
+++ b/libpdbg/i2c.c
@@ -34,7 +34,7 @@  struct i2c_data {
 
 static int i2c_set_addr(int fd, int addr)
 {
-	if (ioctl(fd, I2C_SLAVE, addr) < 0) {
+	if (ioctl(fd, I2C_SLAVE_FORCE, addr) < 0) {
 		PR_ERROR("Unable to set i2c slave address\n");
 		return -1;
 	}