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
