* [PATCH v2] mtd: cmdlinepart: fix the wrong check condition
@ 2012-08-25 20:06 Huang Shijie
2012-08-25 9:31 ` Shmulik Ladkani
0 siblings, 1 reply; 4+ messages in thread
From: Huang Shijie @ 2012-08-25 20:06 UTC (permalink / raw)
To: dwmw2; +Cc: linux-mtd, linux-kernel, dedekind1, Huang Shijie
The `mtd_id` is set by the name of a mtd device driver.
As a nand controller driver, even we do not set the @name of
the mtd_info{}, the nand_get_flash_type() will set it with
the nand type's name. So the `mtd_id` can never be NULL in this
case.
But as a nor controller driver which may does not call the
nand_get_flash_type(), there is a risk that the `mtd_id` becames NULL.
If the `mtd_id` is NULL, the check condition will be true.
If we accidentally set some partitions in the kernel command line,
just like:
#gpmi-nand:20m(boot),20m(kernel),1g(rootfs),-(user)
The cmdlinepart may parses out several mtd partitions right now.
This is obviously wrong. We even do not enable the gpmi-nand in
this case.
The patch comes from Artem's suggestion code which is better then mine.
Signed-off-by: Huang Shijie <shijie8@gmail.com>
---
drivers/mtd/cmdlinepart.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
index fc960a3..216d751 100644
--- a/drivers/mtd/cmdlinepart.c
+++ b/drivers/mtd/cmdlinepart.c
@@ -322,13 +322,16 @@ static int parse_cmdline_partitions(struct mtd_info *master,
struct cmdline_mtd_partition *part;
const char *mtd_id = master->name;
+ if (!mtd_id)
+ return 0;
+
/* parse command line */
if (!cmdline_parsed)
mtdpart_setup_real(cmdline);
for(part = partitions; part; part = part->next)
{
- if ((!mtd_id) || (!strcmp(part->mtd_id, mtd_id)))
+ if (!strcmp(part->mtd_id, mtd_id))
{
for(i = 0, offset = 0; i < part->num_parts; i++)
{
--
1.7.4.4
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v2] mtd: cmdlinepart: fix the wrong check condition
2012-08-25 20:06 [PATCH v2] mtd: cmdlinepart: fix the wrong check condition Huang Shijie
@ 2012-08-25 9:31 ` Shmulik Ladkani
2012-08-26 7:52 ` Huang Shijie
2012-09-02 10:56 ` Artem Bityutskiy
0 siblings, 2 replies; 4+ messages in thread
From: Shmulik Ladkani @ 2012-08-25 9:31 UTC (permalink / raw)
To: Huang Shijie, dedekind1; +Cc: dwmw2, linux-mtd, linux-kernel
Hi Huang, Artem,
On Sat, 25 Aug 2012 16:06:50 -0400 Huang Shijie <shijie8@gmail.com> wrote:
> diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
> index fc960a3..216d751 100644
> --- a/drivers/mtd/cmdlinepart.c
> +++ b/drivers/mtd/cmdlinepart.c
> @@ -322,13 +322,16 @@ static int parse_cmdline_partitions(struct mtd_info *master,
> struct cmdline_mtd_partition *part;
> const char *mtd_id = master->name;
>
> + if (!mtd_id)
> + return 0;
> +
> /* parse command line */
> if (!cmdline_parsed)
> mtdpart_setup_real(cmdline);
>
> for(part = partitions; part; part = part->next)
> {
> - if ((!mtd_id) || (!strcmp(part->mtd_id, mtd_id)))
> + if (!strcmp(part->mtd_id, mtd_id))
> {
> for(i = 0, offset = 0; i < part->num_parts; i++)
> {
This changes the behavior of cmdling parsing, which might affect users
expecting the old behavior.
According to the remark above 'parse_cmdline_partitions':
* It returns partitions for the requested mtd device, or
* the first one in the chain if a NULL mtd_id is passed in.
I think the purpose of a NULL 'mtd_id' was to support simple systems
where there's a single driver and a single chip.
The driver could be dumb, not specifying its 'mtd_info->name'
(thus, a NULL mtd_id is passed).
In this case, since the system is simply configured (one driver, one
chip), 'parse_cmdline_partitions' simply disregards the "mtd-id" name
specified in the cmdline string, allowing the user to present some
arbitrary string there.
I quite remember seeing this pattern somewhere in the past, I don't know
if it's still used, though.
Obviously if you have many drivers (and many chips) in a system, that
won't work; the drivers must initialize 'mtd_info->name' and the user
should present a cmdline that has explicit 'mtd-id's.
So question is, would we like to prohibit NULL mtd-id?
If so, we must make sure all drivers are properly assigning their
'mtd_info->name', and all users correctly specifying 'mtd-id' in their
"mtdparts" cmdline strings.
Regards,
Shmulik
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] mtd: cmdlinepart: fix the wrong check condition
2012-08-25 9:31 ` Shmulik Ladkani
@ 2012-08-26 7:52 ` Huang Shijie
2012-09-02 10:56 ` Artem Bityutskiy
1 sibling, 0 replies; 4+ messages in thread
From: Huang Shijie @ 2012-08-26 7:52 UTC (permalink / raw)
To: Shmulik Ladkani; +Cc: dedekind1, dwmw2, linux-mtd, linux-kernel
On Sat, Aug 25, 2012 at 5:31 AM, Shmulik Ladkani
<shmulik.ladkani@gmail.com> wrote:
> Hi Huang, Artem,
>
> On Sat, 25 Aug 2012 16:06:50 -0400 Huang Shijie <shijie8@gmail.com> wrote:
>> diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
>> index fc960a3..216d751 100644
>> --- a/drivers/mtd/cmdlinepart.c
>> +++ b/drivers/mtd/cmdlinepart.c
>> @@ -322,13 +322,16 @@ static int parse_cmdline_partitions(struct mtd_info *master,
>> struct cmdline_mtd_partition *part;
>> const char *mtd_id = master->name;
>>
>> + if (!mtd_id)
>> + return 0;
>> +
>> /* parse command line */
>> if (!cmdline_parsed)
>> mtdpart_setup_real(cmdline);
>>
>> for(part = partitions; part; part = part->next)
>> {
>> - if ((!mtd_id) || (!strcmp(part->mtd_id, mtd_id)))
>> + if (!strcmp(part->mtd_id, mtd_id))
>> {
>> for(i = 0, offset = 0; i < part->num_parts; i++)
>> {
>
> This changes the behavior of cmdling parsing, which might affect users
> expecting the old behavior.
>
> According to the remark above 'parse_cmdline_partitions':
>
> * It returns partitions for the requested mtd device, or
> * the first one in the chain if a NULL mtd_id is passed in.
>
> I think the purpose of a NULL 'mtd_id' was to support simple systems
> where there's a single driver and a single chip.
> The driver could be dumb, not specifying its 'mtd_info->name'
> (thus, a NULL mtd_id is passed).
>
> In this case, since the system is simply configured (one driver, one
> chip), 'parse_cmdline_partitions' simply disregards the "mtd-id" name
> specified in the cmdline string, allowing the user to present some
> arbitrary string there.
>
> I quite remember seeing this pattern somewhere in the past, I don't know
> if it's still used, though.
>
thanks for the detail explanation.
> Obviously if you have many drivers (and many chips) in a system, that
> won't work; the drivers must initialize 'mtd_info->name' and the user
> should present a cmdline that has explicit 'mtd-id's.
>
> So question is, would we like to prohibit NULL mtd-id?
I prefer to prohibit the NULL mtd-id. The code looks strange enough.
>
> If so, we must make sure all drivers are properly assigning their
> 'mtd_info->name', and all users correctly specifying 'mtd-id' in their
> "mtdparts" cmdline strings.
yes. I agree that all the driver should set the mtd_info->name properly.
Best Regards
Huang Shijie
>
> Regards,
> Shmulik
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] mtd: cmdlinepart: fix the wrong check condition
2012-08-25 9:31 ` Shmulik Ladkani
2012-08-26 7:52 ` Huang Shijie
@ 2012-09-02 10:56 ` Artem Bityutskiy
1 sibling, 0 replies; 4+ messages in thread
From: Artem Bityutskiy @ 2012-09-02 10:56 UTC (permalink / raw)
To: Shmulik Ladkani; +Cc: Huang Shijie, dwmw2, linux-mtd, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1163 bytes --]
On Sat, 2012-08-25 at 12:31 +0300, Shmulik Ladkani wrote:
> Hi Huang, Artem,
>
> On Sat, 25 Aug 2012 16:06:50 -0400 Huang Shijie <shijie8@gmail.com> wrote:
> > diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
> > index fc960a3..216d751 100644
> > --- a/drivers/mtd/cmdlinepart.c
> > +++ b/drivers/mtd/cmdlinepart.c
> > @@ -322,13 +322,16 @@ static int parse_cmdline_partitions(struct mtd_info *master,
> > struct cmdline_mtd_partition *part;
> > const char *mtd_id = master->name;
> >
> > + if (!mtd_id)
> > + return 0;
> > +
> > /* parse command line */
> > if (!cmdline_parsed)
> > mtdpart_setup_real(cmdline);
> >
> > for(part = partitions; part; part = part->next)
> > {
> > - if ((!mtd_id) || (!strcmp(part->mtd_id, mtd_id)))
> > + if (!strcmp(part->mtd_id, mtd_id))
> > {
> > for(i = 0, offset = 0; i < part->num_parts; i++)
> > {
>
> This changes the behavior of cmdling parsing, which might affect users
> expecting the old behavior.
Yes, you are right, we should not change the mtd_id hack unless we have
checked all the users.
--
Best Regards,
Artem Bityutskiy
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-09-02 10:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-25 20:06 [PATCH v2] mtd: cmdlinepart: fix the wrong check condition Huang Shijie
2012-08-25 9:31 ` Shmulik Ladkani
2012-08-26 7:52 ` Huang Shijie
2012-09-02 10:56 ` Artem Bityutskiy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).