Re: [PATCH] arch/tile: add arch/tile/drivers/ directory with SROM driver

[Greg KH <gregkh@suse.de>]

On Fri, May 20, 2011 at 07:39:10PM -0400, Chris Metcalf wrote:
> On 5/20/2011 6:40 PM, Eric Biederman wrote:
> > Please do not make sysfs the direct access method to your device.
> > I may be wrong but I don't think any other driver relies exclusively on sysfs.
> 
> I'm basing my implementation on drivers/misc/eeprom/.  All the drivers
> there use the same sysfs model.
> 
> > Certainly with the introduction of a flush you are introducing an completely
> > different usage paradigm from current users and will need an entirely different
> > set of tools.
> 
> I don't think using my proposed implementation will be detectably different
> for most user tools.  The addition of the flush() method just allows my
> implementation to defer the final sector's write until the device is closed
> (sectors are in fact still written to hardware as one does seek() or
> write() from one sector to another; only the "current" sector is cached by
> the hypervisor).  I suppose some third-party tool that kept the eeprom file
> descriptor open indefinitely and did multiple writes to the same sector
> might not work as expected with this implementation.  But it seems hard to
> imagine a use case for such a tool.
> 
> The direct motivation for this case is to "impedance match" to the
> hypervisor driver for this device, which handles sector management
> internally, so the Linux device doesn't have to.  Having a 'flush' method
> avoids excessive re-writes of the same sector for certain access patterns. 
> The only alternatives that I see are to rewrite the tile userspace tools,
> but they are the way they are because the current model gives good
> consistency guarantees for writing the boot rom in the presence of
> arbitrary failure modes; or, to add something like a delayed timer event
> that allows the Linux driver to notify the hypervisor driver that writes
> are likely complete and it can write out the last sector.  Neither of these
> are particularly attractive.
> 
> > You are vastly exceeding the one value per file rule of sysfs.
> 
> True, but bin_attribute has always been an exception to that rule anyway.

No it hasn't.  A bin attribute is for something that is "one" value, it
is a binary file that the kernel doesn't know anything about, nor does
it intrepret it.

It sounds like you want this binary attribute file to be a bit different
than the "normal" one, and because of that you are wanting to modify the
sysfs file, which proves that you are doing things differently here.

So, I agree with Eric, please do something else as you are not really
wanting to use a binary attribute, you want something else, like a
character driver.

Why can't you do that?

> This driver is a paravirtualized hypervisor driver, so not really an I2C
> driver at all (in fact it handles both SPI and I2C eeproms almost
> identically within the Linux driver).  And I think the driver's "eeprom"
> file should be compatible with userspace cli tools, assuming they close
> their file descriptor when they're done writing.

Big assumption.

> I apologize for not including more of the back story in this email when
> adding the cc's, by the way.

Yeah, that made things difficult :)

> Originally I proposed a straightforward character device for this
> role.

Ah, ok, I think you should do that.

> Arnd Bergmann encouraged me to look at kernel precedents like
> drivers/char/eeprom/, which is why I converted this driver to sysfs.
> The first post in this thread is here:
> https://lkml.org/lkml/2011/5/4/415 .  Since then I've come around to
> believing that it's more valuable to group this driver with the other
> eeprom drivers, rather than with the other paravirtualized tile
> drivers.

See above why I don't think that is so.

thanks,

greg k-h