| Message ID | 20180613064054.20625-3-joel@jms.id.au |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Restore OpenBMC P8 support | expand |
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; > } >
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
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; }