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

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: <4DD6AD51.7090900@tilera.com>
Date:	Fri, 20 May 2011 14:05:05 -0400
From:	Chris Metcalf <cmetcalf@...era.com>
To:	Arnd Bergmann <arnd@...db.de>
CC:	<linux-kernel@...r.kernel.org>,
	"Eric W. Biederman" <ebiederm@...stanetworks.com>,
	Chris Wright <chrisw@...s-sol.org>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Benjamin Thery <benjamin.thery@...l.net>,
	Phil Carmody <ext-phil.2.carmody@...ia.com>
Subject: Re: [PATCH] arch/tile: add arch/tile/drivers/ directory with SROM
 driver

On 5/5/2011 2:41 AM, Arnd Bergmann wrote:
> On Wednesday 04 May 2011, Chris Metcalf wrote:
>> This commit does two things: it adds the infrastructure for a new
>> arch/tile/drivers/ directory, and it populates it with the first
>> instance of a driver for that directory, a SPI Flash ROM (srom) driver.
>>
>> [...]
> I generally advise against putting any device drivers into arch/*/,
> on the ground that it is much easier to find similar drivers when
> they are in a common place. We probably have a few similar drivers
> in the tree already, e.g drivers/misc/eeprom/* and drivers/char/ps3flash.c
> and drivers/platform/x86/intel_scu_ipcutil.c are examples of preexisting
> drivers.
>
> I'd probably put this one in driver/misc/eeprom and make the interface
> look like the other ones (sysfs bin attribute instead of character
> device), which is a trivial change.
>
> [...]
>> The actual SROM driver is fairly uncontroversial, and is just a simple
>> driver that allows user space to read and write the SROM at a raw level.
>> (A separate MTD driver exists for "tile", but this is not that driver.)
>> The driver is particularly useful since the Tile chip can boot directly
>> from the SROM, so providing this driver interface allows for updating
>> the boot image prior to a reboot.
> I think the sysfs bin attribute works well here because you don't need
> any ioctl, and the contents are basically a representation of a flat
> file. The implementation would be almost the same.

This works well, except for the fact that we take advantage of the fact
that the hypervisor driver internally buffers up writes to the current
EEPROM sector, and flushes it to hardware only when explicitly told to do
so, or when we start writing to another sector.  This avoids excessive wear
on the EEPROM and also handles detection of whether we need to do a sector
erase before the re-write.  However, it also means that we need to be able
to issue the final explicit flush, or else the last write just sits in the
hypervisor buffer indefinitely.  To make that happen I think I need to
extend the bin_attribute structure, and the bin.c release() function, slightly:

--- fs/sysfs/bin.c~   2011-05-20 14:02:43.000000000 -0400
+++ fs/sysfs/bin.c  2011-05-20 13:31:23.240866000 -0400
@@ -434,18 +434,26 @@
 }

 static int release(struct inode * inode, struct file * file)
 {
        struct bin_buffer *bb = file->private_data;
+       struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
+       struct bin_attribute *attr = attr_sd->s_bin_attr.bin_attr;
+       int rc = 0;
+
+       if (attr->release && sysfs_get_active(attr_sd)) {
+               rc = attr->release(file, attr_sd->s_parent->s_dir.kobj, attr);
+               sysfs_put_active(attr_sd);
+       }

        mutex_lock(&sysfs_bin_lock);
        hlist_del(&bb->list);
        mutex_unlock(&sysfs_bin_lock);

        kfree(bb->buffer);
        kfree(bb);
-       return 0;
+       return rc;
 }

 const struct file_operations bin_fops = {
        .read           = read,
        .write          = write,
--- include/linux/sysfs.h~   2011-05-20 14:02:43.000000000 -0400
+++ include/linux/sysfs.h   2011-05-20 13:23:57.915080000 -0400
@@ -93,10 +93,11 @@
                        char *, loff_t, size_t);
        ssize_t (*write)(struct file *,struct kobject *, struct bin_attribute *,
                         char *, loff_t, size_t);
        int (*mmap)(struct file *, struct kobject *, struct bin_attribute *attr,
                    struct vm_area_struct *vma);
+       int (*release)(struct file *, struct kobject *, struct bin_attribute *);
 };

 /**
  *     sysfs_bin_attr_init - initialize a dynamically allocated bin_attribute
  *     @attr: struct bin_attribute to initialize



This change shouldn't require any changes to any other bin_attribute users
elsewhere in the kernel.

If something like this seems plausible, I already have the patch ready, and
I can send it to LKML.  I've cc'ed Greg K-H and a few other recent
submitters to bin.c in case they'd like to weigh in at this point too; thanks.

-- 
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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/