Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752421AbbBXIFU (ORCPT ); Tue, 24 Feb 2015 03:05:20 -0500 Received: from mail-pa0-f52.google.com ([209.85.220.52]:47081 "EHLO mail-pa0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751044AbbBXIFR (ORCPT ); Tue, 24 Feb 2015 03:05:17 -0500 Date: Tue, 24 Feb 2015 00:05:12 -0800 From: Brian Norris To: Rodrigo Freire Cc: linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org, =?iso-8859-1?Q?J=F6rn?= Engel , Felix Fietkau , dwmw2@infradead.org, Herton Krzesinski Subject: Re: [PATCH v3 2/3] mtd: block2mtd: Adds a mtd name and a block device timeout option Message-ID: <20150224080512.GE24441@norris-Latitude-E6410> References: <371358190.34795877.1410204429882.JavaMail.zimbra@redhat.com> <1444809468.34812041.1410206680931.JavaMail.zimbra@redhat.com> <20140909170231.GA14429@logfs.org> <1807144344.40128259.1410985683342.JavaMail.zimbra@redhat.com> <20141105202303.GN23619@ld-irv-0074> <2086372266.7454667.1415535533979.JavaMail.zimbra@redhat.com> <2138224172.7454780.1415535747241.JavaMail.zimbra@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2138224172.7454780.1415535747241.JavaMail.zimbra@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5097 Lines: 155 On Sun, Nov 09, 2014 at 07:22:27AM -0500, Rodrigo Freire wrote: > From: Felix Fietkau > > mtd: block2mtd: Adds a mtd name and a block device timeout option > > This patch adds support to a block2mtd MTD name and allows to specify > a block device timeout when adding a block2mtd MTD drive. > Usage: block2mtd=[,[,[,]]] Hmm, are all 4 required in this order? What if I want to set the timeout without the erasesize or name? I suppose it's not a requirement for this patch, but it's probably not hard to handle that as a future enhancement. e.g. block2mtd=][,[][,]]] so I could do block2mtd=/dev/mmcblk0p2,,,4 to set the timeout to 4 seconds. > The devices will be identified the following way: > [root@server01 ~]# cat /proc/mtd > dev: size erasesize name > mtd0: a0000000 00010000 "rootfs" > mtd1: 265080000 00010000 "anothername" > mtd2: acd00000 00010000 "/dev/mmcblk0p2" > Notice that the device mtd2 was added without specifying a name. > > Signed-off-by: Felix Fietkau > Signed-off-by: Rodrigo Freire > Signed-off-by: Herton Krzesinski > --- > V3: Separated on a single patch > --- a/drivers/mtd/devices/block2mtd.c 2014-11-07 17:16:11.711479312 -0200 > +++ b/drivers/mtd/devices/block2mtd.c 2014-11-07 17:15:14.255464054 -0200 > @@ -25,6 +25,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -218,7 +219,7 @@ static void block2mtd_free_device(struct > > > static struct block2mtd_dev *add_device(char *devname, int erase_size, > - int timeout) > + const char *mtdname, int timeout) > { > #ifndef MODULE > int i; > @@ -226,6 +227,7 @@ static struct block2mtd_dev *add_device( > const fmode_t mode = FMODE_READ | FMODE_WRITE | FMODE_EXCL; > struct block_device *bdev = ERR_PTR(-ENODEV); > struct block2mtd_dev *dev; > + struct mtd_partition *part; > char *name; > > if (!devname) > @@ -282,7 +284,9 @@ static struct block2mtd_dev *add_device( > > /* Setup the MTD structure */ > /* make the name contain the block device in */ > - name = kasprintf(GFP_KERNEL, "block2mtd: %s", devname); > + if (!mtdname) > + mtdname = devname; > + name = kstrdup(mtdname, GFP_KERNEL); > if (!name) > goto err_destroy_mutex; > > @@ -301,7 +305,11 @@ static struct block2mtd_dev *add_device( > dev->mtd.priv = dev; > dev->mtd.owner = THIS_MODULE; > > - if (mtd_device_register(&dev->mtd, NULL, 0)) { > + part = kzalloc(sizeof(struct mtd_partition), GFP_KERNEL); sizeof(*part) > + part->name = name; > + part->offset = 0; > + part->size = dev->mtd.size; > + if (mtd_device_register(&dev->mtd, part, 1)) { > /* Device didn't get added, so free the entry */ > goto err_destroy_mutex; > } > @@ -309,8 +317,7 @@ static struct block2mtd_dev *add_device( > list_add(&dev->list, &blkmtd_device_list); > pr_info("mtd%d: [%s] erase_size = %dKiB [%d]\n", > dev->mtd.index, > - dev->mtd.name + strlen("block2mtd: "), > - dev->mtd.erasesize >> 10, dev->mtd.erasesize); > + mtdname, dev->mtd.erasesize >> 10, dev->mtd.erasesize); > return dev; > > err_destroy_mutex: > @@ -383,7 +390,7 @@ static int block2mtd_setup2(const char * > /* 80 for device, 12 for erase size, 80 for name, 8 for timeout */ > char buf[80 + 12 + 80 + 8]; > char *str = buf; > - char *token[2]; > + char *token[4]; > char *name; > size_t erase_size = PAGE_SIZE; > unsigned long timeout = MTD_DEFAULT_TIMEOUT; > @@ -397,7 +404,7 @@ static int block2mtd_setup2(const char * > strcpy(str, val); > kill_final_newline(str); > > - for (i = 0; i < 2; i++) > + for (i = 0; i < 4; i++) Maybe use ARRAY_SIZE(token)? > token[i] = strsep(&str, ","); > > if (str) { > @@ -424,7 +431,13 @@ static int block2mtd_setup2(const char * > } > } > > - add_device(name, erase_size, timeout); > + if (token[2] && (strlen(token[2]) + 1 > 80)) > + pr_err("mtd device name too long"); Hmm, so it's too long, but you continue? Shouldn't we return here? > + > + > + if (token[3] && kstrtoul(token[3], 0, &timeout)) > + pr_err("invalid timeout"); Again, shouldn't we return here? It's best not to just ignore invalid input. > + add_device(name, erase_size, token[2], timeout); > > return 0; > } > @@ -458,7 +471,7 @@ static int block2mtd_setup(const char *v > > > module_param_call(block2mtd, block2mtd_setup, NULL, NULL, 0200); > -MODULE_PARM_DESC(block2mtd, "Device to use. \"block2mtd=[,]\""); > +MODULE_PARM_DESC(block2mtd, "Device to use. \"block2mtd=[,[,[,]]]\""); > > static int __init block2mtd_init(void) > { Brian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/