linux-kernel - Re: [PATCH 1/5] misc: Driver for bh1770glc / sfh7770 ALS and proximity sensor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 05 Oct 2010 15:37:06 +0300
From:	Onkalo Samu <samu.p.onkalo@...ia.com>
To:	ext Alan Cox <alan@...rguk.ukuu.org.uk>
Cc:	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/5] misc: Driver for bh1770glc / sfh7770 ALS and
 proximity sensor

On Tue, 2010-10-05 at 14:01 +0200, ext Alan Cox wrote:
> On Tue, 05 Oct 2010 14:29:50 +0300

> > > > +	for (i = 0; i < ARRAY_SIZE(prox_rates); i++)
> > > > +		if (prox_rates[i] == rate_threshold) {
> > > > +			chip->prox_rate_threshold = i;
> > > > +			return 0;
> > > > +		}
> > > > +
> > > > +	return -EINVAL;
> > > 
> > > This makes it hard for generic code. Wouldn't picking the best (first at
> > > least as good as required) be a bit more polite to user space ?
> > 
> > Well, perhaps it is better to have hardcoded (or platform specific)
> > rates instead of control interface. I'll change that.
> 
> I don't see a problem with a rate setting where you ask for 'at least X'
> and get the nearest ? Ie swap the == for a >= ?

ok.

> 
> > > This makes no sense because you can't poll() a sysfs file
> > 
> > ok, what should be returned when there is no valid results available?
> 
> I have the same problem with the isl29020 - I think you have to block,
> there isn't any other sane response. A user given -EAGAIN simply doesn't
> know what to do
> 

-EIO then the chip is not running and otherwise block until data is
available.

> 
> > > > +static ssize_t bhsfh_lux_calib_show(struct device *dev,
> > > > +				 struct device_attribute *attr, char *buf)
> > > > +{
> > > > +	struct bhsfh_chip *chip = dev_get_drvdata(dev);
> > > > +	return snprintf(buf, PAGE_SIZE, "%u\n", chip->lux_calib);
> > > > +}
> > > 
> > > This is short chip->mutex locks as you sometimes temporarily change the
> > > value (error path below)
> > > 
> > 
> > ?

of course...

> If I am doing a calib store and a calib show in parallel then the show
> can give a bogus answer because of this bit of code
> 
> > > > +	mutex_lock(&chip->mutex);
> > > > +	old_calib = chip->lux_calib;
> > > > +	chip->lux_calib = value;
> > > > +	new_corr = bhsfh_get_corr_value(chip);
> > > > +	if (new_corr == 0) {
> 
> [Show can occur here]
> 
> > > > +		chip->lux_calib = old_calib;
> > > > +		mutex_unlock(&chip->mutex);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +	chip->lux_corr = new_corr;
> > > > +	mutex_unlock(&chip->mutex);
> > > > +
> > > > +	return len;
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/