Re: [RFC PATCHv2 1/4] drivers/otp: add initial support for OTP memory

From: Mike Frysinger
Date: Thu Mar 24 2011 - 14:33:31 EST

On Thu, Mar 24, 2011 at 11:21, Jamie Iles wrote:
> +What: /sys/bus/otp/
> +Description:
> + The otp bus presents a number of devices where each
> + device represents a region or device in the SoC.

is it limited to OTP devices inside of SoCs ? cant OTP devices be on
other busses like I2C or SPI ?

spurious space before "The" ...

> +Contact: Jamie Iles <jamie@xxxxxxxxxxxxx>
> +Contact: "Jamie Iles" <jamie@xxxxxxxxxxxxx>

sometimes you quote and sometimes you dont ... seems like quotes are
useless here

> +What: /sys/bus/otp[a-z]/write_enable

what's with the [a-z] ? how about using otp# like most other people
are doing now ? [a-z] can be a bit limited/confusing ...

> +What: /sys/bus/otp[a-z]/strict_programming
> +Description:
> + This file indicates whether all words in a redundant format
> + must be programmed correctly to indicate success. Disabling
> + this will mean that programming will be considered a success
> + if the word can be read back correctly in it's redundant
> + format.

i dont really grok what this is trying to say ...

"it's" -> "its"

> +What: /sys/bus/otp/devices/otp[a-z][0-9]*/format

you have /sys/bus/otp[a-z]/ and /sys/bus/otp/devices/ ? why not unify them ?

what are each of these subdirs trying to represent ?

> +Description:
> + The redundancy format of the region. Valid values are:
> + - single-ended (1 bit of storage per data bit).
> + - redundant (2 bits of storage, wire-OR'd per data
> + bit).
> + - differential (2 bits of storage, differential
> + voltage per data bit).
> + - differential-redundant (4 bits of storage, combining
> + redundant and differential).
> + It is possible to increase redundancy of a region but care
> + will be needed if there is data already in the region.

where does ECC fit in ? the Blackfin OTP is structured:
- first 4 pages are write control bitfields for all other pages (so
blowing bit 5 of page 0 prevents further writing to page 5)
- each 0x100 page region has 0x20 pages dedicated to ECC for the
other 0x80 pages ... this provides 1 bit error correction and 2 bits
of error detection (anymore and you're screwed!)

> --- /dev/null
> +++ b/drivers/otp/Kconfig
> @@ -0,0 +1,10 @@
> +#
> +# Character device configuration
> +#

old comment

> +menuconfig OTP
> + bool "OTP memory support"
> + help
> + Say y here to support OTP memory found in some embedded devices.
> + This memory can commonly be used to store boot code, cryptographic
> + keys and other persistent data.

is this limited to embedded devices ? i guess TPM keys and such are
already handled by the TPM layers ...

"y" -> "Y"

> --- /dev/null
> +++ b/drivers/otp/otp.c
> +#undef DEBUG

dead code

> +static int otp_open(struct inode *inode, struct file *filp);
> +static int otp_release(struct inode *inode, struct file *filp);
> +static ssize_t otp_write(struct file *filp, const char __user *buf,
> + size_t len, loff_t *offs);
> +static ssize_t otp_read(struct file *filp, char __user *buf, size_t len,
> + loff_t *offs);
> +static loff_t otp_llseek(struct file *filp, loff_t offs, int origin);
> +
> +static const struct file_operations otp_fops = {
> + .owner = THIS_MODULE,
> + .open = otp_open,
> + .release = otp_release,
> + .write = otp_write,
> + .read = otp_read,
> + .llseek = otp_llseek,
> +};

if you moved the fops down to where it is used, you wouldnt need the
redundant func prototypes

> +static DEFINE_SEMAPHORE(otp_sem);
> +static int otp_we, otp_strict_programming;
> +static struct otp_device *otp;
> +static dev_t otp_devno;

hrm, having these be global instead of per-device sounds like a really
bad idea ...

> +/*
> + * Given a device for one of the otpN devices, get the corresponding
> + * otp_region.
> + */
> +static inline struct otp_region *to_otp_region(struct device *dev)
> +{
> + return dev ? container_of(dev, struct otp_region, dev) : NULL;
> +}
> +
> +static inline struct otp_device *to_otp_device(struct device *dev)
> +{
> + return dev ? container_of(dev, struct otp_device, dev) : NULL;
> +}

do you need the NULL checks ? none of the callers of these funcs
check for NULL ...

> +static ssize_t otp_format_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct otp_region *region = to_otp_region(dev);
> + enum otp_redundancy_fmt fmt;
> + const char *fmt_string;
> +
> + if (down_interruptible(&otp_sem))
> + return -ERESTARTSYS;
> +
> + if (region->ops->get_fmt(region))
> + fmt = region->ops->get_fmt(region);
> + else
> + fmt = OTP_REDUNDANCY_FMT_SINGLE_ENDED;
> +
> + up(&otp_sem);

why are you using a semaphore when it looks like you're simply
treating it as a mutex ? make more sense to use a proper mutex
wouldnt it ?

> + if (OTP_REDUNDANCY_FMT_SINGLE_ENDED == fmt)
> + fmt_string = "single-ended";
> + else if (OTP_REDUNDANCY_FMT_REDUNDANT == fmt)
> + fmt_string = "redundant";
> + else if (OTP_REDUNDANCY_FMT_DIFFERENTIAL == fmt)
> + fmt_string = "differential";
> + else if (OTP_REDUNDANCY_FMT_DIFFERENTIAL_REDUNDANT == fmt)
> + fmt_string = "differential-redundant";
> + else
> + fmt_string = NULL;
> +
> + return fmt_string ? sprintf(buf, "%s\n", fmt_string) : -EINVAL;

i'm pretty sure printf in the kernel can handle NULL with %s, so the
NULL check can probably be punted

> +static struct bus_type otp_bus_type = {
> + .name = "otp",
> +};

can this be const ?

> +struct attribute *region_attrs[] = {
> + &dev_attr_format.attr,
> + &dev_attr_size.attr,
> + NULL,
> +};
> +
> +const struct attribute_group region_attr_group = {
> + .attrs = region_attrs,
> +};
> +
> +const struct attribute_group *region_attr_groups[] = {
> + &region_attr_group,
> + NULL,
> +};
> +
> +struct device_type region_type = {
> + .name = "region",
> + .groups = region_attr_groups,
> +};

should these be static ?

> +/*
> + * Show the current write enable state of the OTP. Users can only program the
> + * OTP when this shows 'enabled'.
> + */
> +static ssize_t otp_we_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + int ret;

this func has a ssize_t type but you're using int here. seems to come
up a few times in this patch.

> +/*
> + * Set the write enable state of the OTP. 'enabled' will enable programming
> + * and 'disabled' will prevent further programming from occuring. On loading

"occuring" -> "occurring"

> + * the module, this will default to 'disabled'.
> + */
> +static ssize_t otp_we_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + int err = 0;
> +
> + if (down_interruptible(&otp_sem))
> + return -ERESTARTSYS;
> +
> + if (sysfs_streq(buf, "enabled"))
> + otp_we = 1;
> + else if (sysfs_streq(buf, "disabled"))
> + otp_we = 0;
> + else
> + err = -EINVAL;
> +
> + up(&otp_sem);
> +
> + return err ?: len;
> +}
> +static DEVICE_ATTR(write_enable, S_IRUSR | S_IWUSR, otp_we_show, otp_we_store);

you output and accept "enabled" and "disabled" for multiple values.
how about unifying these ?
return otp_attr_store_enabled(buf, len, &otp_we);

> +static ssize_t otp_num_regions_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int nr_regions;
> +
> + if (down_interruptible(&otp_sem))
> + return -ERESTARTSYS;
> +
> + nr_regions = otp->ops->get_nr_regions(otp);
> +
> + up(&otp_sem);
> +
> + if (nr_regions <= 0)
> + return -EINVAL;
> +
> + return sprintf(buf, "%u\n", nr_regions);
> +}

i'm not sure returning -EINVAL here makes sense ... shouldnt it just
be showing the user the result of get_nr_regions() ?

> +struct attribute *otp_attrs[] = {
> + &dev_attr_strict_programming.attr,
> + &dev_attr_num_regions.attr,
> + &dev_attr_write_enable.attr,
> + NULL,
> +};
> +
> +const struct attribute_group otp_attr_group = {
> + .attrs = otp_attrs,
> +};
> +
> +const struct attribute_group *otp_attr_groups[] = {
> + &otp_attr_group,
> + NULL,
> +};
> +
> +struct device_type otp_type = {
> + .name = "otp",
> + .groups = otp_attr_groups,
> +};

static ?

> +static void otp_dev_release(struct device *dev)
> +{
> + struct otp_device *otp = to_otp_device(dev);
> +
> + kfree(otp);
> + otp = NULL;
> +}

setting to NULL here is pointless when the pointer is on the stack

> +struct otp_device *otp_device_alloc(struct device *dev,
> + const struct otp_device_ops *ops,
> + size_t size)
> +{
> + struct otp_device *otp_dev = NULL;
> + int err = -EINVAL;
> +
> + down(&otp_sem);
> +
> + if (dev && !get_device(dev)) {
> + err = -ENODEV;
> + goto out;
> + }

should you really be allowing dev==NULL ? does that setup make sense ?

> + if (otp) {
> + pr_warning("an otp device already registered\n");
> + err = -EBUSY;
> + goto out_put;
> + }

you can only register one OTP device in the whole system ??

> +void otp_region_unregister(struct otp_region *region)
> +{
> + device_unregister(&region->dev);
> +}
> +EXPORT_SYMBOL_GPL(otp_region_unregister);

wonder if it'd be better to simply #define this in the global otp.h header

> +static ssize_t otp_write(struct file *filp, const char __user *buf, size_t len,
> + loff_t *offs)
> +{
> + unsigned pos = (unsigned)*offs;
> +
> + len = min(len, region->ops->get_size(region) - (unsigned)*offs);

what's with the unsigned cast ? defeats the point of using loff_t doesnt it ?

> + /*
> + * We're now aligned to an 8 byte boundary so we can simply copy words
> + * from userspace and write them into the OTP.
> + */
> + /*
> + * We might have less than 8 bytes left so we'll need to do another
> + * read-modify-write.
> + */
> + while (len >= OTP_WORD_SIZE) {

i think "8 byte" should be "necessary byte" ?

> + if (region->ops->read_word(region, pos / OTP_WORD_SIZE,
> + &word)) {
> + ret = -EIO;
> + goto out;
> + }
> +
> + if (region->ops->write_word(region, pos / OTP_WORD_SIZE,
> + word)) {
> + ret = -EIO;
> + goto out;
> + }

shouldnt you pass the ret value up from read/write word ? this would
allow the lower layers to better describe the issue than just -EIO
wouldnt it ?

last three comments apply to the read func as well ...

> +static int __init otp_init(void)
> +{
> + int err;
> +
> + err = bus_register(&otp_bus_type);
> + if (err)
> + return err;
> +
> + err = alloc_chrdev_region(&otp_devno, 0, 9, "otp");

where'd that magic 9 come from ?

> +MODULE_DESCRIPTION("OTP interface driver");

i think this is also a bus driver ?

> +/*
> + * The OTP works in 64 bit words. When we are programming or reading,
> + * everything is done with 64 bit word addresses.
> + */
> +#define OTP_WORD_SIZE 8

should this be a per-device setting ? or wait until someone who
doesnt have 64bit chunks show up ?

> + * struct otp_device - a picoxcell OTP device.
> + * otp_device_alloc - create a new picoxcell OTP device.

this is no longer picoxcell-specific

> + * otp_device_unregister - deregister an existing struct otp_device.

"deregister" -> "unregister"
-mike
--
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/