[PATCH 1/1] v3.0.x: mtd: check partition count not partition array pointer

[PATCH 1/1] v3.0.x: mtd: check partition count not partition array pointer

[Anthony Foiani]

mtd: check partition count not partition array pointer

The documentation claims that "nr_parts" is the determining factor,
while the code originally tested whether "parts" is non-null.

In at least one driver (fsl_elbc_nand), parts is never initialized to
0; even though nr_parts is correctly 0, add_mtd_partitions still tries
to create 0 partitions.)

Make the code adhere to the documentation.

A quick scan of all uses in the 3.0.51 kernel show that they correctly
rely on nr_parts rather than parts.

The current kernel has retired this function; I have not examined its
replacement to see if it has the same issue.

Signed-Off-By: Anthony Foiani <anthony.foiani@gmail.com>
---
 drivers/mtd/mtdcore.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index c510aff..ac624df 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -446,7 +446,7 @@ int mtd_device_register(struct mtd_info *master,
 			const struct mtd_partition *parts,
 			int nr_parts)
 {
-	return parts ? add_mtd_partitions(master, parts, nr_parts) :
+	return nr_parts ? add_mtd_partitions(master, parts, nr_parts) :
 		add_mtd_device(master);
 }
 EXPORT_SYMBOL_GPL(mtd_device_register);
--
1.7.11.7

Re: [PATCH 1/1] v3.0.x: mtd: check partition count not partition array pointer

[Greg KH]

On Wed, Nov 07, 2012 at 11:48:24PM -0700, Anthony Foiani wrote:
> 
> mtd: check partition count not partition array pointer
> 
> The documentation claims that "nr_parts" is the determining factor,
> while the code originally tested whether "parts" is non-null.
> 
> In at least one driver (fsl_elbc_nand), parts is never initialized to
> 0; even though nr_parts is correctly 0, add_mtd_partitions still tries
> to create 0 partitions.)
> 
> Make the code adhere to the documentation.
> 
> A quick scan of all uses in the 3.0.51 kernel show that they correctly
> rely on nr_parts rather than parts.
> 
> The current kernel has retired this function; I have not examined its
> replacement to see if it has the same issue.
> 
> Signed-Off-By: Anthony Foiani <anthony.foiani@gmail.com>
> ---
>  drivers/mtd/mtdcore.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)


<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
for how to do this properly.

</formletter>

Re: [PATCH 1/1] v3.0.x: mtd: check partition count not partition array pointer

[Anthony Foiani]

Greg KH <greg@kroah.com> writes:
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
> for how to do this properly.

My checklist against stable_kernel_rules.txt is below.

I could only find two reasons why you are saying this is incorrect:

  1. There is no matching upstream commit; or

  2. I did not CC: stable in the signed-off region.

Please let me know which it is, so I can either drop the subject (for
reasons indicated further below), or respin the patch.

Thanks,
Tony

| - It must be obviously correct and tested.

Check.

| - It cannot be bigger than 100 lines, with context.

Check: 1 changed line, 2 active lines in the patch, ~10 lines total.

| - It must fix only one thing.

Check.

| - It must fix a real bug that bothers people (not a, "This could be
|   a problem..." type thing).

It bothered me (made my embedded project not boot correctly).

| - It must fix a problem that causes a build error (but not for
|   things marked CONFIG_BROKEN), an oops, a hang, data corruption, a
|   real security issue, or some "oh, that's not good" issue.  In
|   short, something critical.

It repaired a change in behavior; for my project that was a "oh,
that's not good" issue.

It booted, but in such a way that it failed to expose a critical
system resource correctly.

| - Serious issues as reported by a user of a distribution kernel [...]

Not applicable.

| - New device IDs and quirks are also accepted.

Not applicable.

| - No "theoretical race condition" issues [...]

Not applicable.

| - It cannot contain any "trivial" fixes in it [...]

Check.

| - It must follow the Documentation/SubmittingPatches rules.

As far as I can tell, this patch follows those rules. 

| - It or an equivalent fix must already exist in Linus' tree (upstream).

As mentioned in the original submission:

>> The current kernel has retired this function; I have not examined
>> its replacement to see if it has the same issue.

You can either use this 1-line patch, or you can have someone else
backport the changes made in the mainline kernel.  That someone will
not be me, sadly.

| Procedure for submitting patches to the -stable tree:
|
| - Send the patch, after verifying that it follows the above rules,
|   to stable@vger.kernel.org.

I thought I did this, but I'm guessing that you're complaining about:

|   You must note the upstream commit ID in the changelog of your
|   submission.

As mentioned above, there is no corresponding upstream commit, because
that function got removed.

I thought that my contribution was more in the spirit of the stable
series ("small, obvious, correct"); more so than would be the
backporting of the upstream fix.

But it's your call; if you disagree, then you disagree.  My patch will
be here if other people need it.

| - To have the patch automatically included in the stable tree, add
|   the tag
|
|     Cc: stable@vger.kernel.org
|
|   in the sign-off area. Once the patch is merged it will be applied
|   to the stable tree without anything else needing to be done by the
|   author or subsystem maintainer.

I would be happy to resubmit with that modification, if you find the
other aspects of the patch acceptable.

With only the formletter response, I'm unable to determine which bits
you dislike.

| - If the patch requires other patches as prerequisites which can be
|   cherry-picked than this can be specified in the following format
|   [...]

Not applicable

| - The sender will receive an ACK when the patch has been accepted
|   into the queue, or a NAK if the patch is rejected.  This response
|   might take a few days, according to the developer's schedules.

I received a NAK, and it was timely enough -- thank you.  :)

| - If accepted, the patch will be added to the -stable queue, for
|   review by other developers and by the relevant subsystem
|   maintainer.

Presumably not applicable until you tell me why it was rejected; if it
is repairable, I'll be happy to resubmit.

| - Security patches should not be sent to this alias, but instead to the
|   documented security@kernel.org address.

Not applicable.

...and I'll not belabor the point.

Re: [PATCH 1/1] v3.0.x: mtd: check partition count not partition array pointer

[Greg KH]

On Fri, Nov 30, 2012 at 05:19:28PM -0700, Anthony Foiani wrote:
> Greg KH <greg@kroah.com> writes:
> > This is not the correct way to submit patches for inclusion in the
> > stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
> > for how to do this properly.
> 
> My checklist against stable_kernel_rules.txt is below.
> 
> I could only find two reasons why you are saying this is incorrect:
> 
>   1. There is no matching upstream commit; or

This is an obvious one, it needs to be upstream first.

Or if not, a whole lot of explaination saying that you know it isn't,
and why it isn't, and why it isn't applicable there, including agreement
from the subsystem maintainers saying that they agree with you.

thanks,

greg k-h

Re: [PATCH 1/1] v3.0.x: mtd: check partition count not partition array pointer

[Anthony Foiani]

Greg --

Thanks for the very quick response.

Greg KH <greg@kroah.com> writes:
> This is an obvious one, it needs to be upstream first.
>
> Or if not, a whole lot of explaination saying that you know it
> isn't, and why it isn't, and why it isn't applicable there,

I thought that I did provide exactly this information, when I
indicated that this function no longer even existed in upstream.

The replacement in upstream was to fold this functionality into
another function -- but I found that function much more complex and
difficult to follow.

So I went with the simple fix.

> including agreement from the subsystem maintainers saying that they
> agree with you.

I also don't have that.  I might post it to the mtd list, to see if
they have any interest in either approving it as-is for 3.0 series, or
doing the backport of the current code.

Thanks again for your time.

Tony