Re: [PATCH v2 1/2] hwmon: (pmbus/isl68137) add debugfs config and black box endpoints

[Guenter Roeck]

On Mon, Apr 27, 2020 at 12:07:29PM -0500, Grant Peltier wrote:
> On Sat, Apr 25, 2020 at 10:43:18AM -0700, Guenter Roeck wrote:
> > On Wed, Apr 22, 2020 at 12:27:14PM -0500, Grant Peltier wrote:
> > > Add debugfs endpoints to support features of 2nd generation Renesas
> > > digital multiphase voltage regulators that are not compatible with
> > > sysfs.
> > > 
> > > The read_black_box endpoint allows users to read the contents of a
> > > RAM segment used to record fault conditions within Gen 2 devices.
> > > 
> > > The write_config endpoint allows users to write configuration hex
> > > files to Gen 2 devices which modify how they operate.
> > > 
> > > Signed-off-by: Grant Peltier <grantpeltier93@xxxxxxxxx>
> > 
> > Comments inline.
> > 
> > However, the more I look into this, the more concerns I have.
> > I think we should limit debugfs functions, if they are needed,
> > to reporting detailed device status. Can you consider handling
> > configuration from userspace using i2cget / i2cset commands ?
> 
> The reason we decided to try to implement configuration writes within the
> driver is that we found a userspace implementation to be unstable. The
> process requires anywhere from approximately 650 to a few thousand 32-bit
> writes (dependent on number of NVM slots contained in the file). The entire
> write process therefore takes a non-trivial amount of CPU time to complete
> and the userspace process was often interrupted which would cause for it
> to fail. Writing the configuration directly from the driver has been less
> error prone.
> 
The downside is that anyone can easily mess with the chip. That does make
me quite concerned - I have seen many PMBus chips and even boards blown up
if misconfigured. It is bad enough that this can be done in the first place,
but I don't want to make it even easier by providing the means to do so via
debugfs files.

Did you try with an application sending ioctls directly ? That should
be much less error prone than i2cset/i2cget. Also, a userspace process
like this should have appropriate priorities (ie run as realtime process)
to avoid being preempted.

Thanks,
Guenter

> > > +	res = i2c_smbus_read_i2c_block_data(ctrl->client, PMBUS_IC_DEVICE_REV,
> > > +					    5, dev_rev);
> > 
> > It still puzzles me, quite frankly, why i2c_smbus_read_block_data()
> > would not work here.
> >
> 
> i2c_smbus_read_block_data() requires the underlying driver/controller to handle
> interpretting the initial length byte read from the client device and then
> continuing to read that number of bytes. Not all controllers (e.g. BCM2835)
> support this. On the other hand, i2c_smbus_read_i2c_block_data() just does a
> fixed-length read based on the given length parameter.
> 
> > > +static int raa_dmpvr2_cfg_write_result(struct raa_dmpvr2_ctrl *ctrl,
> > > +				       struct raa_dmpvr2_cfg *cfg)
> > > +{
> > > +	u8 data[4] = {0};
> > > +	u8 data1[4];
> > > +	u8 *dptr;
> > > +	unsigned long start;
> > > +	int i, j, status;
> > > +
> > > +	// Check programmer status
> > > +	start = jiffies;
> > > +	i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
> > > +				  RAA_DMPVR2_PRGM_STATUS_ADDR);
> > > +	while (data[0] == 0 && !time_after(jiffies, start + HZ + HZ)) {
> > > +		raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_FIX,
> > > +					data);
> > > +	}
> > > +	if (data[0] != 1)
> > > +		return -ETIME;
> > 
> > Are you sure ? Normally I would expect ETIMEDOUT.
>  
> My understanding is that ETIME is meant for timer expiration whereas ETIMEDOUT
> is meant for connection timeout errors. Is that correct? In this case, we are
> not really waiting on the device to respond but instead are constantly polling
> until the device responds with the desired value. However, I can understand an
> argument for ETIMEDOUT here and can swtich to that if you think it is more
> appropriate.
> 
> 
> Thank you for your other notes. I will refactor as requested.
> 
> Grant