RE: [PATCH 2/2] mtd: cmdlinepart: use cmdline partition parser lib
> Such results was never posted and unless we see those, I think > I'd rather NACK this patch instead. I like the cleanup, but only > if it's guaranteed to _not_ brake things, specially when dealing > with a kernel parameter. Do you have some test case or test standard for me perform. > -- > Ezequiel García, Free Electrons > Embedded Linux, Kernel and Android Engineering > http://free-electrons.com
Re: [PATCH 2/2] mtd: cmdlinepart: use cmdline partition parser lib
On Fri, Nov 08, 2013 at 06:53:29AM +, Caizhiyong wrote: > >> For further information, see "https://lkml.org/lkml/2013/8/6/550; > > > > Thanks for doing this. Could we please get some acked-by's or, > > preferably, tested-by's from the MTD people? > > Acked-by: Ezequiel Garcia I don't remember acking this patch! Instead, I do remember asking for the test results, prooving the this change has _no_ change of behavior compared to the MTD parsing code: https://lkml.org/lkml/2013/10/25/164 Such results was never posted and unless we see those, I think I'd rather NACK this patch instead. I like the cleanup, but only if it's guaranteed to _not_ brake things, specially when dealing with a kernel parameter. -- Ezequiel García, Free Electrons Embedded Linux, Kernel and Android Engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] mtd: cmdlinepart: use cmdline partition parser lib
On Fri, Nov 08, 2013 at 06:53:29AM +, Caizhiyong wrote: For further information, see https://lkml.org/lkml/2013/8/6/550; Thanks for doing this. Could we please get some acked-by's or, preferably, tested-by's from the MTD people? Acked-by: Ezequiel Garcia ezequiel.gar...@free-electrons.com I don't remember acking this patch! Instead, I do remember asking for the test results, prooving the this change has _no_ change of behavior compared to the MTD parsing code: https://lkml.org/lkml/2013/10/25/164 Such results was never posted and unless we see those, I think I'd rather NACK this patch instead. I like the cleanup, but only if it's guaranteed to _not_ brake things, specially when dealing with a kernel parameter. -- Ezequiel García, Free Electrons Embedded Linux, Kernel and Android Engineering http://free-electrons.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] mtd: cmdlinepart: use cmdline partition parser lib
Such results was never posted and unless we see those, I think I'd rather NACK this patch instead. I like the cleanup, but only if it's guaranteed to _not_ brake things, specially when dealing with a kernel parameter. Do you have some test case or test standard for me perform. -- Ezequiel García, Free Electrons Embedded Linux, Kernel and Android Engineering http://free-electrons.com
RE: [PATCH 2/2] mtd: cmdlinepart: use cmdline partition parser lib
> Nobody has had time to test this on MTD, it seems, and as such, I > strongly recommend you do not force it through -mm. We are perfectly > capable of merging it through the MTD tree if it ever gets proper > vetting by people in MTD (not just on block devices), and I am well > aware of this patch set's existence. > > However, the patch really provides little to no benefit to the MTD > subsystem at the moment, at the added cost of reviewing the small > differences in parsing. For some reason, Cai decided to make small > differences in the parser between drivers/mtd/cmdlinepart.c and > block/cmdline-parser.c, and it is not obvious that these differences > retain the same parsing. For instance, according to my code > read-through, the block device parser seems to accept multiple > partitions to be specified with "-" (meaning "fill the remaining > device"). MTD already has code that rejects such a parser string > outright. The next '-' partition be ignore at function "cmdline_parts_set", and the client will get a right result. Is there any other worry about '-' I don't know ? > > So, I'd recommend one of the following: > (1) We (MTD users) make more time to properly test this patch and post > relevant results (i.e., tested partition strings) or > (2) Somebody (Cai?) spend time to actually make block/cmdline-parser.c > fully equivalent to the parser in drivers/mtd/cmdlinepart.c. That > means it should be trivial to compare the two and say "yes, these are > equivalent". That is currently not the case, AFAICT. I understand your worry about, we use cmdlinepart many years. I will spend time to make block/cmdline-parser.c fully equivalent to the parser in drivers/mtd/cmdlinepart.c. > > Without one of those two, I will give my NAK. > > Brian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] mtd: cmdlinepart: use cmdline partition parser lib
>> For further information, see "https://lkml.org/lkml/2013/8/6/550; > > Thanks for doing this. Could we please get some acked-by's or, > preferably, tested-by's from the MTD people? Acked-by: Ezequiel Garcia Acked-by: Andrew Morton -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] mtd: cmdlinepart: use cmdline partition parser lib
For further information, see https://lkml.org/lkml/2013/8/6/550; Thanks for doing this. Could we please get some acked-by's or, preferably, tested-by's from the MTD people? Acked-by: Ezequiel Garcia ezequiel.gar...@free-electrons.com Acked-by: Andrew Morton a...@linux-foundation.org -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2] mtd: cmdlinepart: use cmdline partition parser lib
Nobody has had time to test this on MTD, it seems, and as such, I strongly recommend you do not force it through -mm. We are perfectly capable of merging it through the MTD tree if it ever gets proper vetting by people in MTD (not just on block devices), and I am well aware of this patch set's existence. However, the patch really provides little to no benefit to the MTD subsystem at the moment, at the added cost of reviewing the small differences in parsing. For some reason, Cai decided to make small differences in the parser between drivers/mtd/cmdlinepart.c and block/cmdline-parser.c, and it is not obvious that these differences retain the same parsing. For instance, according to my code read-through, the block device parser seems to accept multiple partitions to be specified with - (meaning fill the remaining device). MTD already has code that rejects such a parser string outright. The next '-' partition be ignore at function cmdline_parts_set, and the client will get a right result. Is there any other worry about '-' I don't know ? So, I'd recommend one of the following: (1) We (MTD users) make more time to properly test this patch and post relevant results (i.e., tested partition strings) or (2) Somebody (Cai?) spend time to actually make block/cmdline-parser.c fully equivalent to the parser in drivers/mtd/cmdlinepart.c. That means it should be trivial to compare the two and say yes, these are equivalent. That is currently not the case, AFAICT. I understand your worry about, we use cmdlinepart many years. I will spend time to make block/cmdline-parser.c fully equivalent to the parser in drivers/mtd/cmdlinepart.c. Without one of those two, I will give my NAK. Brian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] mtd: cmdlinepart: use cmdline partition parser lib
On Tue, Nov 5, 2013 at 2:43 PM, Andrew Morton wrote: > On Tue, 22 Oct 2013 13:14:17 + Caizhiyong > wrote: > >> In the previous version, adjust the cmdline parser code to library-style >> code, and move it to a separate file "block/cmdline-parser.c", we can use >> it in some client code. there is no any functionality change in the >> adjusting. >> >> this patch use cmdline parser lib. >> >> For further information, see "https://lkml.org/lkml/2013/8/6/550; > > Thanks for doing this. Could we please get some acked-by's or, > preferably, tested-by's from the MTD people? Nobody has had time to test this on MTD, it seems, and as such, I strongly recommend you do not force it through -mm. We are perfectly capable of merging it through the MTD tree if it ever gets proper vetting by people in MTD (not just on block devices), and I am well aware of this patch set's existence. However, the patch really provides little to no benefit to the MTD subsystem at the moment, at the added cost of reviewing the small differences in parsing. For some reason, Cai decided to make small differences in the parser between drivers/mtd/cmdlinepart.c and block/cmdline-parser.c, and it is not obvious that these differences retain the same parsing. For instance, according to my code read-through, the block device parser seems to accept multiple partitions to be specified with "-" (meaning "fill the remaining device"). MTD already has code that rejects such a parser string outright. So, I'd recommend one of the following: (1) We (MTD users) make more time to properly test this patch and post relevant results (i.e., tested partition strings) or (2) Somebody (Cai?) spend time to actually make block/cmdline-parser.c fully equivalent to the parser in drivers/mtd/cmdlinepart.c. That means it should be trivial to compare the two and say "yes, these are equivalent". That is currently not the case, AFAICT. Without one of those two, I will give my NAK. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] mtd: cmdlinepart: use cmdline partition parser lib
On Tue, 22 Oct 2013 13:14:17 + Caizhiyong wrote: > In the previous version, adjust the cmdline parser code to library-style > code, and move it to a separate file "block/cmdline-parser.c", we can use > it in some client code. there is no any functionality change in the adjusting. > > this patch use cmdline parser lib. > > For further information, see "https://lkml.org/lkml/2013/8/6/550; Thanks for doing this. Could we please get some acked-by's or, preferably, tested-by's from the MTD people? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] mtd: cmdlinepart: use cmdline partition parser lib
On Tue, 22 Oct 2013 13:14:17 + Caizhiyong caizhiy...@hisilicon.com wrote: In the previous version, adjust the cmdline parser code to library-style code, and move it to a separate file block/cmdline-parser.c, we can use it in some client code. there is no any functionality change in the adjusting. this patch use cmdline parser lib. For further information, see https://lkml.org/lkml/2013/8/6/550; Thanks for doing this. Could we please get some acked-by's or, preferably, tested-by's from the MTD people? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] mtd: cmdlinepart: use cmdline partition parser lib
On Tue, Nov 5, 2013 at 2:43 PM, Andrew Morton a...@linux-foundation.org wrote: On Tue, 22 Oct 2013 13:14:17 + Caizhiyong caizhiy...@hisilicon.com wrote: In the previous version, adjust the cmdline parser code to library-style code, and move it to a separate file block/cmdline-parser.c, we can use it in some client code. there is no any functionality change in the adjusting. this patch use cmdline parser lib. For further information, see https://lkml.org/lkml/2013/8/6/550; Thanks for doing this. Could we please get some acked-by's or, preferably, tested-by's from the MTD people? Nobody has had time to test this on MTD, it seems, and as such, I strongly recommend you do not force it through -mm. We are perfectly capable of merging it through the MTD tree if it ever gets proper vetting by people in MTD (not just on block devices), and I am well aware of this patch set's existence. However, the patch really provides little to no benefit to the MTD subsystem at the moment, at the added cost of reviewing the small differences in parsing. For some reason, Cai decided to make small differences in the parser between drivers/mtd/cmdlinepart.c and block/cmdline-parser.c, and it is not obvious that these differences retain the same parsing. For instance, according to my code read-through, the block device parser seems to accept multiple partitions to be specified with - (meaning fill the remaining device). MTD already has code that rejects such a parser string outright. So, I'd recommend one of the following: (1) We (MTD users) make more time to properly test this patch and post relevant results (i.e., tested partition strings) or (2) Somebody (Cai?) spend time to actually make block/cmdline-parser.c fully equivalent to the parser in drivers/mtd/cmdlinepart.c. That means it should be trivial to compare the two and say yes, these are equivalent. That is currently not the case, AFAICT. Without one of those two, I will give my NAK. Brian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
