Re: [PATCH linux 2/6] hwmon: occ: Add sysfs interface

[Andrew Jeffery <andrew@aj.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4017 bytes --]

On Sat, 2017-01-07 at 09:15 -0800, Guenter Roeck wrote:
> On 01/06/2017 02:17 PM, Edward James wrote:
> 
> [ ... ]
> 
> > > > +}
> > > > +
> > > > +static DEVICE_ATTR(online, S_IWUSR | S_IRUGO, show_occ_online,
> > > > +         store_occ_online);
> > > > +
> > > > +struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
> > > > +              struct occ_sysfs_config *config)
> > > > +{
> > > > +   struct occ_sysfs *hwmon = devm_kzalloc(dev, sizeof(struct occ_sysfs),
> > > > +                      GFP_KERNEL);
> > > > +   int rc;
> > > > +
> > > > +   if (!hwmon)
> > > > +      return ERR_PTR(-ENOMEM);
> > > > +
> > > > +   hwmon->occ = occ;
> > > > +   hwmon->num_caps_fields = config->num_caps_fields;
> > > > +   hwmon->caps_names = config->caps_names;
> > > > +
> > > > +   dev_set_drvdata(dev, hwmon);
> > > > +
> > > > +   rc = device_create_file(dev, &dev_attr_online);
> > > > +   if (rc)
> > > > +      return ERR_PTR(rc);
> > > > +
> > > > +   return hwmon;
> > > > +}
> > > > +EXPORT_SYMBOL(occ_sysfs_start);
> > > > +
> > > > +int occ_sysfs_stop(struct device *dev, struct occ_sysfs *driver)
> > > > +{
> > > > +   if (driver->dev) {
> > > > +      occ_remove_hwmon_attrs(driver);
> > > > +      hwmon_device_unregister(driver->dev);
> > > > +   }
> > > > +
> > > > +   device_remove_file(driver->dev, &dev_attr_online);
> > > > +
> > > > +   devm_kfree(dev, driver);
> > > 
> > > Thw point of using devm_ functions is not to require remove/free functions.
> > > Something is completely wrong here if you need that call.
> > > 
> > > Overall, this is architectually completely wrong. One does not register
> > > or instantiate drivers based on writing into sysfs attributes. Please
> > > reconsider your approach.
> > 
> > We had some trouble designing this driver because the BMC only has
> > access to the OCC once the processor is powered on. This will happen
> > sometime after the BMC boots (this driver runs only on the BMC). With
> > no access to the OCC, we don't know what sensors are present on the
> > system without a large static enumeration. Also any sysfs files created
> > before we have OCC access won't be able to return any data.
> > 
> > Instead of the "online" attribute, what do you think about using the
> > "bind"/"unbind" API to probe the device from user space once the system
> > is powered on? All the hwmon registration would take place in the probe
> > function, it would just occur some time after boot.
> > 
> 
> A more common approach would be to have a platform driver. That platform
> driver would need a means to detect if the OCC is up and running, and
> instantiate everything else once it is.
> 
> A trigger from user space is problematic because there is no guarantee
> that the OCC is really up (or that it even exists).

This is true in general, but for the BMC case we have more information:
The host CPU power supply is controlled by several GPIOs from
userspace. Once we receive the "power-good" signal for the host CPU we
can bind the OCC driver and trigger the probe.

Alternatively, in the style of your first para, we could push the host
CPU state management into the kernel and expose a boot/reboot/power-off 
API to userspace. That would give us a place to hook calls for
configuring and cleaning up any host-dependent drivers on the BMC.

The solution to the host-power-state problem is also applicable to the
OpenFSI patches that were recently sent out:

https://lkml.org/lkml/2016/12/6/732

The OpenFSI infra needs to re-scan for CFAMs when the host is powered
up.

> 
> An alternative might be to have the hwmon driver poll for the OCC,
> but that would be a bit more difficult and might require a kernel thread
> or maybe asynchronous probing.

This was our thought as a fallback solution.

Andrew

> 
> Guenter
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]