Re: [PATCH v2 3/4] hwmon: (adt7x10) Add alarm interrupt support

[Guenter Roeck <linux@roeck-us.net>]

On Tue, Feb 19, 2013 at 01:05:42PM +0100, Lars-Peter Clausen wrote:
> On 02/19/2013 02:39 AM, Guenter Roeck wrote:
> > On Mon, Feb 18, 2013 at 02:38:58PM +0100, Lars-Peter Clausen wrote:
> >> This allows a userspace application to poll() on the alarm files to get notified
> >> in case of an temperature threshold event.
> >>
> >> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> >>
> [...]
> >>  
> >> +static irqreturn_t adt7410_irq_handler(int irq, void *private)
> >> +{
> >> +	struct device *dev = private;
> >> +	int status;
> >> +
> >> +	status = adt7410_read_byte(dev, ADT7410_STATUS);
> >> +	if (status < 0)
> >> +		return IRQ_HANDLED;
> >> +
> >> +	if (status & ADT7410_STAT_T_HIGH)
> >> +		sysfs_notify(&dev->kobj, NULL, "temp1_max_alarm");
> >> +	if (status & ADT7410_STAT_T_LOW)
> >> +		sysfs_notify(&dev->kobj, NULL, "temp1_min_alarm");
> >> +	if (status & ADT7410_STAT_T_CRIT)
> >> +		sysfs_notify(&dev->kobj, NULL, "temp1_crit_alarm");
> >> +
> > Question about semantics: Do you want a notification on all attributes each time
> > one is set during an interrupt, or do you want a notification each time an
> > attribute changes state ? With the above code, the 1->0 transition does not
> > result in a notification. Is that on purpose ?
> > 
> 
> Now that the code uses an edge triggered IRQ and comparator mode it would
> actually be possible to also generate an event for a 1->0 transition, but
> I'm not sure how much sense that makes. Usually you'd only want to respond
> quickly to the case where a threshold event happens.
> 
I am not sure if it would make sense either. I am fine either way - this is
quite new functionality for hwmon, and we can add support for two-way
notification later if it turns out to be needed.

> >> +	return IRQ_HANDLED;
> >> +}
> >> +
> >>  static int adt7410_temp_ready(struct device *dev)
> >>  {
> >>  	int i, status;
> >> @@ -357,7 +377,7 @@ static const struct attribute_group adt7410_group = {
> >>  	.attrs = adt7410_attributes,
> >>  };
> >>  
> >> -int adt7410_probe(struct device *dev, const char *name,
> >> +int adt7410_probe(struct device *dev, const char *name, int irq,
> >>  	const struct adt7410_ops *ops)
> >>  {
> >>  	struct adt7410_data *data;
> >> @@ -383,11 +403,13 @@ int adt7410_probe(struct device *dev, const char *name,
> >>  	data->oldconfig = ret;
> >>  
> >>  	/*
> >> -	 * Set to 16 bit resolution, continous conversion and comparator mode.
> >> +	 * Set to 16 bit resolution and continous conversion
> >>  	 */
> >>  	data->config = data->oldconfig;
> >> -	data->config &= ~ADT7410_MODE_MASK;
> >> +	data->config &= ~(ADT7410_MODE_MASK | ADT7410_CT_POLARITY |
> >> +			ADT7410_INT_POLARITY);
> >>  	data->config |= ADT7410_FULL | ADT7410_RESOLUTION | ADT7410_EVENT_MODE;
> >> +
> >>  	if (data->config != data->oldconfig) {
> >>  		ret = adt7410_write_byte(dev, ADT7410_CONFIG, data->config);
> >>  		if (ret)
> >> @@ -421,8 +443,18 @@ int adt7410_probe(struct device *dev, const char *name,
> >>  		goto exit_remove_name;
> >>  	}
> >>  
> >> +	if (irq > 0) {
> >> +		ret = request_threaded_irq(irq, NULL, adt7410_irq_handler,
> >> +				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> >> +				dev_name(dev), dev);
> > 
> > If you use devm_request_threaded_irq, you don't have to release it on remove.
> 
> devm_request_threaded_irq is tricky, since the IRQ will only be freed after
> the drivers remove callback has been run. This can cause race conditions,
> since the IRQ handler often access resources already freed in the remove
> callback. In this case it will work fine since sysfs_notify handles the case
> where the files are already gone gracefully and just does nothing in that
> case. I'd still prefer to keep the explicit free, so things are cleaned up
> in the reverse order to the way they were set up.
> 
Ok, makes sense.

> > 
> >> +		if (ret)
> >> +			goto exit_hwmon_device_unregister;
> >> +	}
> >> +
> > 
> > This code should be before hwmon registration.
> 
> Why? It doesn't make sense to signal an event before userspace can actually
> discover the device.
> 
You are right. Historically we keep hwmon registration as last operation to
avoid user-space confusion if the attributes are gone by the time user-space
tries to access them. Getting a notification prior to registration is just as
bad, and would be "normal" operation instead of error handling, so your code
makes sense.

Thanks,
Guenter