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

[Chris Metcalf]

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.

> Please look at the i2c core and if at all possible build a proper interface to
> your eeprom, that existing programs have a chance of utilizing rather than
> extending sysfs in ways that means cli tools can not work with it.

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.

I apologize for not including more of the back story in this email when
adding the cc's, by the way.  Originally I proposed a straightforward
character device for this role.  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.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com


--
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/