Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Oliver Schinagl]

On 06/24/13 20:15, Greg KH wrote:
> On Mon, Jun 24, 2013 at 07:11:35PM +0200, Oliver Schinagl wrote:
> > Hey Greg,
> > On 06/24/13 18:04, Greg KH wrote:
> > > On Mon, Jun 24, 2013 at 11:29:42AM +0200, Maxime Ripard wrote:
> > > > Hi Greg,
> > > >
> > > > On Mon, Jun 17, 2013 at 03:58:47PM -0700, Greg KH wrote:
> > > > > On Mon, Jun 17, 2013 at 10:59:37PM +0200, Oliver Schinagl wrote:
> > > >
> > > > [..]
> > > >
> > > > > > +static int __init sunxi_sid_probe(struct platform_device *pdev)
> > > > > > +{
> > > > > > +	u8 entropy[SID_SIZE];
> > > > > > +	unsigned int i;
> > > > > > +	struct resource *res;
> > > > > > +	void __iomem *sid_reg_base;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > > > +	sid_reg_base = devm_ioremap_resource(&pdev->dev, res);
> > > > > > +	if (IS_ERR(sid_reg_base))
> > > > > > +		return PTR_ERR(sid_reg_base);
> > > > > > +	platform_set_drvdata(pdev, sid_reg_base);
> > > > > > +
> > > > > > +	ret = device_create_bin_file(&pdev->dev, &sid_bin_attr);
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > >
> > > > > You just raced with userspace, having the file show up after the device
> > > > > was announced to users that it was there.  Please use the proper device
> > > > > file api to add default attributes to prevent this from happening.
> > > >
> > > > Sorry if the question looks dumb, but what kind of race can we generate
> > > > here?
> > >
> > > Userspace gets told about the device from the driver core, udev runs and
> > > reads all of the attributes, then your probe function comes along and
> > > adds a new attribute.  Userspace will then not know about it at all.
> > >
> > > > The device_create_bin_file is the last call that we make (if we except
> > > > the entropy stuff, but it doesn't really matter here), so after we
> > > > created the file, we have everything properly initialised so that our
> > > > functions can be called, right?
> > > >
> > > > And another dumb question for you, what is the "proper device file API"
> > > > you are referring to ? :)
> > >
> > > Please read Documentation/driver_model/device.txt and see the section on
> > > Attributes for what to do.  If you have specific questions after reading
> > > that, please let me know.
> > Since Maxime kinda asked for me, I hope you don't mind me following up.
> >
> > That doc doesn't mention the binary interface at all. Initially I
> > had both devices up, the 'read' device as a textual representation
> > and added the binary one later. Maxime and I decided the binary one
> > made more sense, as the only textual user would be a human and they
> > don't poke that entry that often.
> >
> > So what default way exists for binary files or how would that be solved?
>
> The same interface should work just fine for binary files, have you
> tried it?
I'll just take the plunge and make myself look stupid ;)

I tried to change things around, used DEVICE_ATTR(eeprom, S_IRUGO, 
sid_read, NULL); So far so good I'd hope.

Of course now I'll have to change the function's parameters from

static ssize_t sid_read(struct file *fd, struct kobject *kobj,
			struct bin_attribute *attr, char *buf,
			loff_t pos, size_t size)

to

static ssize_t sid_read(struct device *dev,
			struct device_attribute *attr, char *buf)

But now, I'm missing things like 'pos' and 'size', both which determine 
the requested bytes. True, in this specific driver we are talking about 
'only' 16 bytes, but what if it weren't but a few MiB and in sysfs we 
want to read some random byte, will we have to put the entire blok into 
the buffer?

So sorry for not understanding, but ... I don't understand :)

Oliver
> thanks,
>
> greg k-h

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