linux-kernel - Re: [PATCH] misc: adds support the FSA9480 USB Switch

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]

Message-ID: <20100917224416.GB22684@kroah.com>
Date:	Fri, 17 Sep 2010 15:44:16 -0700
From:	Greg KH <greg@...ah.com>
To:	Andrew Morton <akpm@...ux-foundation.org>,
	Minkyu Kang <mk7.kang@...sung.com>,
	linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org,
	kyungmin.park@...sung.com
Subject: Re: [PATCH] misc: adds support the FSA9480 USB Switch

On Fri, Sep 17, 2010 at 03:07:31PM -0700, Andrew Morton wrote:
> > +static ssize_t fsa9480_show_status(struct device *dev,
> > +				   struct device_attribute *attr,
> > +				   char *buf)
> > +{
> > +	struct fsa9480_usbsw *usbsw = dev_get_drvdata(dev);
> > +	struct i2c_client *client = usbsw->client;
> > +	int devid, ctrl, adc, dev1, dev2, intr,
> > +	    intmask1, intmask2, time1, time2, mansw1;
> > +
> > +	devid = fsa9480_read_reg(client, FSA9480_REG_DEVID);
> > +	ctrl = fsa9480_read_reg(client, FSA9480_REG_CTRL);
> > +	adc = fsa9480_read_reg(client, FSA9480_REG_ADC);
> > +	intmask1 = fsa9480_read_reg(client, FSA9480_REG_INT1_MASK);
> > +	intmask2 = fsa9480_read_reg(client, FSA9480_REG_INT2_MASK);
> > +	dev1 = fsa9480_read_reg(client, FSA9480_REG_DEV_T1);
> > +	dev2 = fsa9480_read_reg(client, FSA9480_REG_DEV_T2);
> > +	time1 = fsa9480_read_reg(client, FSA9480_REG_TIMING1);
> > +	time2 = fsa9480_read_reg(client, FSA9480_REG_TIMING2);
> > +	mansw1 = fsa9480_read_reg(client, FSA9480_REG_MANSW1);
> > +
> > +	fsa9480_read_irq(client, &intr);
> > +
> > +	return sprintf(buf, "Device ID(%02x), CTRL(%02x)\n"
> > +			"ADC(%02x), DEV_T1(%02x), DEV_T2(%02x)\n"
> > +			"INT(%04x), INTMASK(%02x, %02x)\n"
> > +			"TIMING(%02x, %02x), MANSW1(%02x)\n",
> > +			devid, ctrl, adc, dev1, dev2, intr,
> > +			intmask1, intmask2, time1, time2, mansw1);
> > +}
> 
> That's will produce odd-looking output I suspect.  More conventional
> would be
> 
> 	Device ID:%02x CTRL:%02x
> 
> or something like that.
> 
> But that result is basically unparseable by software and a better output
> would be
> 
> Device_ID: %02x
> CTRL: %02x
> ADC: %02x
> 
> etc.
> 
> But even that violates the sysfs one-value-per-file guideline.
> 
> So this interface is problematic.  It should have been discussed
> up-front in the changelog so we can all take a look at the proposal and
> have a think about it.

Yes, such a sysfs file is not allowed, please just have individual sysfs
files for these attribute types (device id, control, adc, etc.)

And are you sure they are even needed?  Some of those are already known
through the i2c interface, right?

thanks,

greg k-h
--
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/