Re: [PATCH 06/44 take 2] [UBI] startup code
Christoph, On Thu, 2007-05-17 at 15:44 +0100, Christoph Hellwig wrote: > So both I and Rusty told you not to do this and you did it anyway. > > I'm quite pissed about this ignorance. Andrew, what do we do about such > a case? Should we just revert ubi until they fixed their mess up? Just for reference: http://lkml.org/lkml/2007/2/19/67 Sorry, I thought Rusty meant that we can use it if we invent our own type, see the first part of his answer. You did not answer Frank's mail about _why_ we use it. So I thought you agreed with us, I did not ignore you, no need to be so assertive. It is not a problem to fix this, but neither Rusty nor you explained what is wrong with that call. But we _did_ explain why we use it. You ignored the explanation and the questions. -- Best regards, Artem Bityutskiy (Битюцкий Артём) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/44 take 2] [UBI] startup code
On Sun, Feb 25, 2007 at 05:58:28AM +, Christoph Hellwig wrote: > > Why not? We tried to avoid this but found out that this is the most > > decent interface. Specific advises are welcome. > > because this type of compount interface is really painful for the user. > the module.param=foo syntax makes sure paramaters can be used without > endless documentation for each and every single of them, and makes > sure module writers don't introduce bugs in their own parser > reimplementations. > > Rusty, was it intentional that drivers can use __module_param_call? > Do you think the ubi use here is okay? So both I and Rusty told you not to do this and you did it anyway. I'm quite pissed about this ignorance. Andrew, what do we do about such a case? Should we just revert ubi until they fixed their mess up? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/44 take 2] [UBI] startup code
On Sun, Feb 25, 2007 at 05:58:28AM +, Christoph Hellwig wrote: Why not? We tried to avoid this but found out that this is the most decent interface. Specific advises are welcome. because this type of compount interface is really painful for the user. the module.param=foo syntax makes sure paramaters can be used without endless documentation for each and every single of them, and makes sure module writers don't introduce bugs in their own parser reimplementations. Rusty, was it intentional that drivers can use __module_param_call? Do you think the ubi use here is okay? So both I and Rusty told you not to do this and you did it anyway. I'm quite pissed about this ignorance. Andrew, what do we do about such a case? Should we just revert ubi until they fixed their mess up? - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/44 take 2] [UBI] startup code
Christoph, On Thu, 2007-05-17 at 15:44 +0100, Christoph Hellwig wrote: So both I and Rusty told you not to do this and you did it anyway. I'm quite pissed about this ignorance. Andrew, what do we do about such a case? Should we just revert ubi until they fixed their mess up? Just for reference: http://lkml.org/lkml/2007/2/19/67 Sorry, I thought Rusty meant that we can use it if we invent our own type, see the first part of his answer. You did not answer Frank's mail about _why_ we use it. So I thought you agreed with us, I did not ignore you, no need to be so assertive. It is not a problem to fix this, but neither Rusty nor you explained what is wrong with that call. But we _did_ explain why we use it. You ignored the explanation and the questions. -- Best regards, Artem Bityutskiy (Битюцкий Артём) - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/44 take 2] [UBI] startup code
Hi Rusty, On Mon, 2007-02-26 at 09:03 +1100, Rusty Russell wrote: > On Sun, 2007-02-25 at 05:58 +, Christoph Hellwig wrote: > > On Tue, Feb 20, 2007 at 03:00:56PM +0200, Artem Bityutskiy wrote: > > > > > +module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000); > > > > > +MODULE_PARM_DESC(mtd, "MTD devices to attach. Parameter format: " > > > > > + "mtd=[,,]. " > > > > > + "Multiple \"mtd\" parameters may be specified.\n" > > > > > + "MTD devices may be specified by their number or > > > > > name. " > > > > > + "Optional \"vid_hdr_offs\" and \"data_offs\" > > > > > parameters " > > > > > + "specify UBI VID header position and data > > > > > starting " > > > > > + "position to be used by UBI.\n" > > > > > + "Example: mtd=content,1984,2048 mtd=4 - attach > > > > > MTD device" > > > > > + "with name content using VID header offset 1984 > > > > > and data " > > > > > + "start 2048, and MTD device number 4 using > > > > > default " > > > > > + "offsets"); > > > > > The reason drivers can use __module_param_call is that they can > implement their own "types" for module parameters, which will end up > requiring this. > > Using it directly is only really for backwards compatibility (which is > important!), but for new parameters, this multi-part self-parsing is > nasty. Standard (but admittedly suboptimal) way to do this is having > three parameters module_param_array(name, ...), > module_param_array(header_offset, ...), > module_param_array(data_start, ...). we wanted to be able to ommit some of the parameters and let UBI use resonable defaults if needed. Example where we wanted to explicitly overwrite the offset parameters: mtd=content,1984,2048 Example where we did not want it but let instead UBI figure out resonable parameters: mtd=4 With the three arrays we did not see a nice way of achieving this. ubimtds=content,4,7 hdroffs=1984,?,1984 dataoff=s2048,?,2048 The ? look odd, don't they? We also looked at the slram parameters which are like this: slram=name0,addr0,+size0,name1,addr1,+size1 and found that if we wanted to add one more parameter e.g. width it cause likely an interface change: slram=name0,addr0,+size0,width0,name1,addr1,+size1,width1 Finally we found that we liked the phram parameter parsing (which is essentially the same) and did it similar to that. Frank - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/44 take 2] [UBI] startup code
Hi Rusty, On Mon, 2007-02-26 at 09:03 +1100, Rusty Russell wrote: On Sun, 2007-02-25 at 05:58 +, Christoph Hellwig wrote: On Tue, Feb 20, 2007 at 03:00:56PM +0200, Artem Bityutskiy wrote: +module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000); +MODULE_PARM_DESC(mtd, MTD devices to attach. Parameter format: + mtd=name|num[,vid_hdr_offs,data_offs]. + Multiple \mtd\ parameters may be specified.\n + MTD devices may be specified by their number or name. + Optional \vid_hdr_offs\ and \data_offs\ parameters + specify UBI VID header position and data starting + position to be used by UBI.\n + Example: mtd=content,1984,2048 mtd=4 - attach MTD device + with name content using VID header offset 1984 and data + start 2048, and MTD device number 4 using default + offsets); The reason drivers can use __module_param_call is that they can implement their own types for module parameters, which will end up requiring this. Using it directly is only really for backwards compatibility (which is important!), but for new parameters, this multi-part self-parsing is nasty. Standard (but admittedly suboptimal) way to do this is having three parameters module_param_array(name, ...), module_param_array(header_offset, ...), module_param_array(data_start, ...). we wanted to be able to ommit some of the parameters and let UBI use resonable defaults if needed. Example where we wanted to explicitly overwrite the offset parameters: mtd=content,1984,2048 Example where we did not want it but let instead UBI figure out resonable parameters: mtd=4 With the three arrays we did not see a nice way of achieving this. ubimtds=content,4,7 hdroffs=1984,?,1984 dataoff=s2048,?,2048 The ? look odd, don't they? We also looked at the slram parameters which are like this: slram=name0,addr0,+size0,name1,addr1,+size1 and found that if we wanted to add one more parameter e.g. width it cause likely an interface change: slram=name0,addr0,+size0,width0,name1,addr1,+size1,width1 Finally we found that we liked the phram parameter parsing (which is essentially the same) and did it similar to that. Frank - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/44 take 2] [UBI] startup code
On Sun, 2007-02-25 at 05:58 +, Christoph Hellwig wrote: > because this type of compount interface is really painful for the user. > the module.param=foo syntax makes sure paramaters can be used without > endless documentation for each and every single of them, and makes > sure module writers don't introduce bugs in their own parser > reimplementations. I stand that without the use this function the interface is much worse. I will be grateful if you advice something better. -- Best regards, Artem Bityutskiy (Битюцкий Артём) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/44 take 2] [UBI] startup code
On Sun, 2007-02-25 at 05:58 +, Christoph Hellwig wrote: because this type of compount interface is really painful for the user. the module.param=foo syntax makes sure paramaters can be used without endless documentation for each and every single of them, and makes sure module writers don't introduce bugs in their own parser reimplementations. I stand that without the use this function the interface is much worse. I will be grateful if you advice something better. -- Best regards, Artem Bityutskiy (Битюцкий Артём) - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/44 take 2] [UBI] startup code
On Sun, 2007-02-25 at 05:58 +, Christoph Hellwig wrote: > On Tue, Feb 20, 2007 at 03:00:56PM +0200, Artem Bityutskiy wrote: > > > > +module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000); > > > > +MODULE_PARM_DESC(mtd, "MTD devices to attach. Parameter format: " > > > > + "mtd=[,,]. " > > > > + "Multiple \"mtd\" parameters may be specified.\n" > > > > + "MTD devices may be specified by their number or > > > > name. " > > > > + "Optional \"vid_hdr_offs\" and \"data_offs\" > > > > parameters " > > > > + "specify UBI VID header position and data > > > > starting " > > > > + "position to be used by UBI.\n" > > > > + "Example: mtd=content,1984,2048 mtd=4 - attach > > > > MTD device" > > > > + "with name content using VID header offset 1984 > > > > and data " > > > > + "start 2048, and MTD device number 4 using > > > > default " > > > > + "offsets"); > > > > > > This is a very odd paramater interface. We really don't want drivers to > > > use > > > module_param_call directly. You probably want various module_param_array > > > calls > > > instead. > > > > Why not? We tried to avoid this but found out that this is the most > > decent interface. Specific advises are welcome. > > because this type of compount interface is really painful for the user. > the module.param=foo syntax makes sure paramaters can be used without > endless documentation for each and every single of them, and makes > sure module writers don't introduce bugs in their own parser > reimplementations. > > Rusty, was it intentional that drivers can use __module_param_call? > Do you think the ubi use here is okay? The reason drivers can use __module_param_call is that they can implement their own "types" for module parameters, which will end up requiring this. Using it directly is only really for backwards compatibility (which is important!), but for new parameters, this multi-part self-parsing is nasty. Standard (but admittedly suboptimal) way to do this is having three parameters module_param_array(name, ...), module_param_array(header_offset, ...), module_param_array(data_start, ...). Hope that helps, Rusty. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/44 take 2] [UBI] startup code
On Sun, 2007-02-25 at 05:58 +, Christoph Hellwig wrote: On Tue, Feb 20, 2007 at 03:00:56PM +0200, Artem Bityutskiy wrote: +module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000); +MODULE_PARM_DESC(mtd, MTD devices to attach. Parameter format: + mtd=name|num[,vid_hdr_offs,data_offs]. + Multiple \mtd\ parameters may be specified.\n + MTD devices may be specified by their number or name. + Optional \vid_hdr_offs\ and \data_offs\ parameters + specify UBI VID header position and data starting + position to be used by UBI.\n + Example: mtd=content,1984,2048 mtd=4 - attach MTD device + with name content using VID header offset 1984 and data + start 2048, and MTD device number 4 using default + offsets); This is a very odd paramater interface. We really don't want drivers to use module_param_call directly. You probably want various module_param_array calls instead. Why not? We tried to avoid this but found out that this is the most decent interface. Specific advises are welcome. because this type of compount interface is really painful for the user. the module.param=foo syntax makes sure paramaters can be used without endless documentation for each and every single of them, and makes sure module writers don't introduce bugs in their own parser reimplementations. Rusty, was it intentional that drivers can use __module_param_call? Do you think the ubi use here is okay? The reason drivers can use __module_param_call is that they can implement their own types for module parameters, which will end up requiring this. Using it directly is only really for backwards compatibility (which is important!), but for new parameters, this multi-part self-parsing is nasty. Standard (but admittedly suboptimal) way to do this is having three parameters module_param_array(name, ...), module_param_array(header_offset, ...), module_param_array(data_start, ...). Hope that helps, Rusty. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/44 take 2] [UBI] startup code
On Tue, Feb 20, 2007 at 03:00:56PM +0200, Artem Bityutskiy wrote: > > > +module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000); > > > +MODULE_PARM_DESC(mtd, "MTD devices to attach. Parameter format: " > > > + "mtd=[,,]. " > > > + "Multiple \"mtd\" parameters may be specified.\n" > > > + "MTD devices may be specified by their number or name. " > > > + "Optional \"vid_hdr_offs\" and \"data_offs\" parameters " > > > + "specify UBI VID header position and data starting " > > > + "position to be used by UBI.\n" > > > + "Example: mtd=content,1984,2048 mtd=4 - attach MTD device" > > > + "with name content using VID header offset 1984 and data " > > > + "start 2048, and MTD device number 4 using default " > > > + "offsets"); > > > > This is a very odd paramater interface. We really don't want drivers to use > > module_param_call directly. You probably want various module_param_array > > calls > > instead. > > Why not? We tried to avoid this but found out that this is the most > decent interface. Specific advises are welcome. because this type of compount interface is really painful for the user. the module.param=foo syntax makes sure paramaters can be used without endless documentation for each and every single of them, and makes sure module writers don't introduce bugs in their own parser reimplementations. Rusty, was it intentional that drivers can use __module_param_call? Do you think the ubi use here is okay? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/44 take 2] [UBI] startup code
On Tue, Feb 20, 2007 at 03:00:56PM +0200, Artem Bityutskiy wrote: +module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000); +MODULE_PARM_DESC(mtd, MTD devices to attach. Parameter format: + mtd=name|num[,vid_hdr_offs,data_offs]. + Multiple \mtd\ parameters may be specified.\n + MTD devices may be specified by their number or name. + Optional \vid_hdr_offs\ and \data_offs\ parameters + specify UBI VID header position and data starting + position to be used by UBI.\n + Example: mtd=content,1984,2048 mtd=4 - attach MTD device + with name content using VID header offset 1984 and data + start 2048, and MTD device number 4 using default + offsets); This is a very odd paramater interface. We really don't want drivers to use module_param_call directly. You probably want various module_param_array calls instead. Why not? We tried to avoid this but found out that this is the most decent interface. Specific advises are welcome. because this type of compount interface is really painful for the user. the module.param=foo syntax makes sure paramaters can be used without endless documentation for each and every single of them, and makes sure module writers don't introduce bugs in their own parser reimplementations. Rusty, was it intentional that drivers can use __module_param_call? Do you think the ubi use here is okay? - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/44 take 2] [UBI] startup code
On Tue, 2007-02-20 at 15:00 +0200, Artem Bityutskiy wrote: > > This looks very odd. > > What exactly? I see, will be fixed, thanks. -- Best regards, Artem Bityutskiy (Битюцкий Артём) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/44 take 2] [UBI] startup code
On Tue, 2007-02-20 at 15:00 +0200, Artem Bityutskiy wrote: This looks very odd. What exactly? I see, will be fixed, thanks. -- Best regards, Artem Bityutskiy (Битюцкий Артём) - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/44 take 2] [UBI] startup code
On Mon, 2007-02-19 at 10:59 +, Christoph Hellwig wrote:
> On Sat, Feb 17, 2007 at 06:54:54PM +0200, Artem Bityutskiy wrote:
> > +/* UBI headers must take 64 bytes. The below is a hacky way to ensure this
> > */
> > +static int __ubi_check_ec_hdr_size[(UBI_EC_HDR_SIZE == 64) - 1]
> > +__attribute__ ((__unused__));
> > +static int __ubi_check_ec_hdr_size[(UBI_VID_HDR_SIZE == 64) - 1]
> > +__attribute__ ((__unused__));
>
> please use BUILD_BUG_ON instead.
Will be done, thanks.
> > +
> > +static int ubi_attach_mtd_dev(const char *mtd_dev, int vid_hdr_offset,
> > + int data_offset);
> > +static void ubi_destroy_dev(int ubi_num);
>
> Can you reorder the code to avoid all these forward declarations please?
Could you please submit a CodingStyle patch that would contain a
requirement to use the "higher-level functions at the bottom,
lower-layer at top"? Because I just use the opposite.
> > + /* Attach MTD devices */
> > + for (i = 0; i < mtd_devs; i++) {
> > + struct mtd_dev_param *p = _dev_param[i];
> > +
> > + cond_resched();
> > + err = -EINVAL;
>
> This looks very odd.
What exactly?
> > +module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000);
> > +MODULE_PARM_DESC(mtd, "MTD devices to attach. Parameter format: "
> > + "mtd=[,,]. "
> > + "Multiple \"mtd\" parameters may be specified.\n"
> > + "MTD devices may be specified by their number or name. "
> > + "Optional \"vid_hdr_offs\" and \"data_offs\" parameters "
> > + "specify UBI VID header position and data starting "
> > + "position to be used by UBI.\n"
> > + "Example: mtd=content,1984,2048 mtd=4 - attach MTD device"
> > + "with name content using VID header offset 1984 and data "
> > + "start 2048, and MTD device number 4 using default "
> > + "offsets");
>
> This is a very odd paramater interface. We really don't want drivers to use
> module_param_call directly. You probably want various module_param_array
> calls
> instead.
Why not? We tried to avoid this but found out that this is the most
decent interface. Specific advises are welcome.
Thank you,
Artem.
--
Best regards,
Artem Bityutskiy (Битюцкий Артём)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/44 take 2] [UBI] startup code
On Mon, 2007-02-19 at 10:59 +, Christoph Hellwig wrote:
On Sat, Feb 17, 2007 at 06:54:54PM +0200, Artem Bityutskiy wrote:
+/* UBI headers must take 64 bytes. The below is a hacky way to ensure this
*/
+static int __ubi_check_ec_hdr_size[(UBI_EC_HDR_SIZE == 64) - 1]
+__attribute__ ((__unused__));
+static int __ubi_check_ec_hdr_size[(UBI_VID_HDR_SIZE == 64) - 1]
+__attribute__ ((__unused__));
please use BUILD_BUG_ON instead.
Will be done, thanks.
+
+static int ubi_attach_mtd_dev(const char *mtd_dev, int vid_hdr_offset,
+ int data_offset);
+static void ubi_destroy_dev(int ubi_num);
Can you reorder the code to avoid all these forward declarations please?
Could you please submit a CodingStyle patch that would contain a
requirement to use the higher-level functions at the bottom,
lower-layer at top? Because I just use the opposite.
+ /* Attach MTD devices */
+ for (i = 0; i mtd_devs; i++) {
+ struct mtd_dev_param *p = mtd_dev_param[i];
+
+ cond_resched();
+ err = -EINVAL;
This looks very odd.
What exactly?
+module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000);
+MODULE_PARM_DESC(mtd, MTD devices to attach. Parameter format:
+ mtd=name|num[,vid_hdr_offs,data_offs].
+ Multiple \mtd\ parameters may be specified.\n
+ MTD devices may be specified by their number or name.
+ Optional \vid_hdr_offs\ and \data_offs\ parameters
+ specify UBI VID header position and data starting
+ position to be used by UBI.\n
+ Example: mtd=content,1984,2048 mtd=4 - attach MTD device
+ with name content using VID header offset 1984 and data
+ start 2048, and MTD device number 4 using default
+ offsets);
This is a very odd paramater interface. We really don't want drivers to use
module_param_call directly. You probably want various module_param_array
calls
instead.
Why not? We tried to avoid this but found out that this is the most
decent interface. Specific advises are welcome.
Thank you,
Artem.
--
Best regards,
Artem Bityutskiy (Битюцкий Артём)
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/44 take 2] [UBI] startup code
On Sat, Feb 17, 2007 at 06:54:54PM +0200, Artem Bityutskiy wrote:
> +/* UBI headers must take 64 bytes. The below is a hacky way to ensure this */
> +static int __ubi_check_ec_hdr_size[(UBI_EC_HDR_SIZE == 64) - 1]
> +__attribute__ ((__unused__));
> +static int __ubi_check_ec_hdr_size[(UBI_VID_HDR_SIZE == 64) - 1]
> +__attribute__ ((__unused__));
please use BUILD_BUG_ON instead.
> +
> +static int ubi_attach_mtd_dev(const char *mtd_dev, int vid_hdr_offset,
> + int data_offset);
> +static void ubi_destroy_dev(int ubi_num);
Can you reorder the code to avoid all these forward declarations please?
> + /* Attach MTD devices */
> + for (i = 0; i < mtd_devs; i++) {
> + struct mtd_dev_param *p = _dev_param[i];
> +
> + cond_resched();
> + err = -EINVAL;
This looks very odd.
> +module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000);
> +MODULE_PARM_DESC(mtd, "MTD devices to attach. Parameter format: "
> + "mtd=[,,]. "
> + "Multiple \"mtd\" parameters may be specified.\n"
> + "MTD devices may be specified by their number or name. "
> + "Optional \"vid_hdr_offs\" and \"data_offs\" parameters "
> + "specify UBI VID header position and data starting "
> + "position to be used by UBI.\n"
> + "Example: mtd=content,1984,2048 mtd=4 - attach MTD device"
> + "with name content using VID header offset 1984 and data "
> + "start 2048, and MTD device number 4 using default "
> + "offsets");
This is a very odd paramater interface. We really don't want drivers to use
module_param_call directly. You probably want various module_param_array calls
instead.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/44 take 2] [UBI] startup code
On Sat, Feb 17, 2007 at 06:54:54PM +0200, Artem Bityutskiy wrote:
+/* UBI headers must take 64 bytes. The below is a hacky way to ensure this */
+static int __ubi_check_ec_hdr_size[(UBI_EC_HDR_SIZE == 64) - 1]
+__attribute__ ((__unused__));
+static int __ubi_check_ec_hdr_size[(UBI_VID_HDR_SIZE == 64) - 1]
+__attribute__ ((__unused__));
please use BUILD_BUG_ON instead.
+
+static int ubi_attach_mtd_dev(const char *mtd_dev, int vid_hdr_offset,
+ int data_offset);
+static void ubi_destroy_dev(int ubi_num);
Can you reorder the code to avoid all these forward declarations please?
+ /* Attach MTD devices */
+ for (i = 0; i mtd_devs; i++) {
+ struct mtd_dev_param *p = mtd_dev_param[i];
+
+ cond_resched();
+ err = -EINVAL;
This looks very odd.
+module_param_call(mtd, ubi_mtd_param_parse, NULL, NULL, 000);
+MODULE_PARM_DESC(mtd, MTD devices to attach. Parameter format:
+ mtd=name|num[,vid_hdr_offs,data_offs].
+ Multiple \mtd\ parameters may be specified.\n
+ MTD devices may be specified by their number or name.
+ Optional \vid_hdr_offs\ and \data_offs\ parameters
+ specify UBI VID header position and data starting
+ position to be used by UBI.\n
+ Example: mtd=content,1984,2048 mtd=4 - attach MTD device
+ with name content using VID header offset 1984 and data
+ start 2048, and MTD device number 4 using default
+ offsets);
This is a very odd paramater interface. We really don't want drivers to use
module_param_call directly. You probably want various module_param_array calls
instead.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 06/44 take 2] [UBI] startup code
diff -auNrp tmp-from/drivers/mtd/ubi/init.c tmp-to/drivers/mtd/ubi/init.c
--- tmp-from/drivers/mtd/ubi/init.c 1970-01-01 02:00:00.0 +0200
+++ tmp-to/drivers/mtd/ubi/init.c 2007-02-17 18:07:26.0 +0200
@@ -0,0 +1,371 @@
+/*
+ * Copyright (c) International Business Machines Corp., 2006
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
+ * the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * Author: Artem B. Bityutskiy,
+ * Frank Haverkamp
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "ubi.h"
+#include "alloc.h"
+#include "uif.h"
+#include "io.h"
+#include "build.h"
+#include "debug.h"
+
+/* Maximum MTD device specification parameter length */
+#define UBI_MTD_PARAM_LEN_MAX 64
+
+/**
+ * struct mtd_dev_param - MTD device parameter description data structure.
+ *
+ * @name: MTD device name or number string
+ * @vid_hdr_offs: VID header offset
+ * @data_offs: data offset
+ */
+struct mtd_dev_param
+{
+ char name[UBI_MTD_PARAM_LEN_MAX];
+ int vid_hdr_offs;
+ int data_offs;
+};
+
+/* Numbers of elements set in the @mtd_dev_param array. */
+static int mtd_devs = 0;
+
+/* MTD devices specification parameters */
+static struct mtd_dev_param mtd_dev_param[UBI_MAX_INSTANCES];
+
+/* Number of UBI devices in system */
+int ubis_num;
+
+/* All the UBI devices in system */
+struct ubi_info *ubis[UBI_MAX_INSTANCES];
+
+/* UBI headers must take 64 bytes. The below is a hacky way to ensure this */
+static int __ubi_check_ec_hdr_size[(UBI_EC_HDR_SIZE == 64) - 1]
+__attribute__ ((__unused__));
+static int __ubi_check_ec_hdr_size[(UBI_VID_HDR_SIZE == 64) - 1]
+__attribute__ ((__unused__));
+
+static int ubi_attach_mtd_dev(const char *mtd_dev, int vid_hdr_offset,
+ int data_offset);
+static void ubi_destroy_dev(int ubi_num);
+
+static int __init ubi_init(void)
+{
+ int err, i, k;
+
+ if (mtd_devs > UBI_MAX_INSTANCES) {
+ printk("UBI error: too many MTD devices, max. is %d\n",
+ UBI_MAX_INSTANCES);
+ return -EINVAL;
+ }
+
+ err = ubi_dbg_init();
+ if (err) {
+ printk("UBI error: failed to initialize debugging unit, "
+ "error %d", err);
+ return err;
+ }
+
+ err = ubi_alloc_init();
+ if (err) {
+ dbg_err("failed to initialize memory allocation unit, "
+ "error %d", err);
+ goto out_dbg;
+ }
+
+ /* Initialize the user interface unit */
+ err = ubi_uif_global_init();
+ if (err) {
+ dbg_err("failed to initialize user interfaces unit, error %d",
+ err);
+ goto out_alloc;
+ }
+
+ /* Attach MTD devices */
+ for (i = 0; i < mtd_devs; i++) {
+ struct mtd_dev_param *p = _dev_param[i];
+
+ cond_resched();
+ err = -EINVAL;
+
+ /* First suppose this is MTD device name */
+ err = ubi_attach_mtd_dev(p->name, p->vid_hdr_offs,
+p->data_offs);
+ if (err)
+ goto out_detach;
+ }
+
+ return 0;
+
+out_detach:
+ for (k = 0; k < i; k++)
+ ubi_destroy_dev(k);
+ ubi_uif_global_close();
+out_alloc:
+ ubi_alloc_close();
+out_dbg:
+ ubi_dbg_close();
+ return err;
+}
+module_init(ubi_init);
+
+static void __exit ubi_exit(void)
+{
+ int i;
+
+ for (i = 0; i < ubis_num; i++)
+ ubi_destroy_dev(i);
+ ubi_uif_global_close();
+ ubi_alloc_close();
+ ubi_dbg_close();
+}
+module_exit(ubi_exit);
+
+/**
+ * ubi_attach_mtd_dev - attach an MTD device.
+ *
+ * @mtd_dev: MTD device name or number string to attach
+ * @vid_hdr_offset: volume identifier header offset in physical eraseblocks
+ * @data_offset: data offset in physical eraseblock
+ *
+ * This function attaches an MTD device to UBI. It first treats @mtd_dev as the
+ * MTD device name, and tries to open it by this name. If it is unable to open,
+ * it tries to convert @mtd_dev to an integer and open the MTD device by its
+ * number. Returns zero in case of success and a negative error code in case of
+ * failure.
+ */
[PATCH 06/44 take 2] [UBI] startup code
diff -auNrp tmp-from/drivers/mtd/ubi/init.c tmp-to/drivers/mtd/ubi/init.c
--- tmp-from/drivers/mtd/ubi/init.c 1970-01-01 02:00:00.0 +0200
+++ tmp-to/drivers/mtd/ubi/init.c 2007-02-17 18:07:26.0 +0200
@@ -0,0 +1,371 @@
+/*
+ * Copyright (c) International Business Machines Corp., 2006
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
+ * the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * Author: Artem B. Bityutskiy,
+ * Frank Haverkamp
+ */
+
+#include linux/init.h
+#include linux/err.h
+#include linux/module.h
+#include linux/moduleparam.h
+#include linux/sched.h
+#include linux/stringify.h
+#include linux/types.h
+#include linux/stat.h
+#include mtd/ubi-header.h
+#include ubi.h
+#include alloc.h
+#include uif.h
+#include io.h
+#include build.h
+#include debug.h
+
+/* Maximum MTD device specification parameter length */
+#define UBI_MTD_PARAM_LEN_MAX 64
+
+/**
+ * struct mtd_dev_param - MTD device parameter description data structure.
+ *
+ * @name: MTD device name or number string
+ * @vid_hdr_offs: VID header offset
+ * @data_offs: data offset
+ */
+struct mtd_dev_param
+{
+ char name[UBI_MTD_PARAM_LEN_MAX];
+ int vid_hdr_offs;
+ int data_offs;
+};
+
+/* Numbers of elements set in the @mtd_dev_param array. */
+static int mtd_devs = 0;
+
+/* MTD devices specification parameters */
+static struct mtd_dev_param mtd_dev_param[UBI_MAX_INSTANCES];
+
+/* Number of UBI devices in system */
+int ubis_num;
+
+/* All the UBI devices in system */
+struct ubi_info *ubis[UBI_MAX_INSTANCES];
+
+/* UBI headers must take 64 bytes. The below is a hacky way to ensure this */
+static int __ubi_check_ec_hdr_size[(UBI_EC_HDR_SIZE == 64) - 1]
+__attribute__ ((__unused__));
+static int __ubi_check_ec_hdr_size[(UBI_VID_HDR_SIZE == 64) - 1]
+__attribute__ ((__unused__));
+
+static int ubi_attach_mtd_dev(const char *mtd_dev, int vid_hdr_offset,
+ int data_offset);
+static void ubi_destroy_dev(int ubi_num);
+
+static int __init ubi_init(void)
+{
+ int err, i, k;
+
+ if (mtd_devs UBI_MAX_INSTANCES) {
+ printk(UBI error: too many MTD devices, max. is %d\n,
+ UBI_MAX_INSTANCES);
+ return -EINVAL;
+ }
+
+ err = ubi_dbg_init();
+ if (err) {
+ printk(UBI error: failed to initialize debugging unit,
+ error %d, err);
+ return err;
+ }
+
+ err = ubi_alloc_init();
+ if (err) {
+ dbg_err(failed to initialize memory allocation unit,
+ error %d, err);
+ goto out_dbg;
+ }
+
+ /* Initialize the user interface unit */
+ err = ubi_uif_global_init();
+ if (err) {
+ dbg_err(failed to initialize user interfaces unit, error %d,
+ err);
+ goto out_alloc;
+ }
+
+ /* Attach MTD devices */
+ for (i = 0; i mtd_devs; i++) {
+ struct mtd_dev_param *p = mtd_dev_param[i];
+
+ cond_resched();
+ err = -EINVAL;
+
+ /* First suppose this is MTD device name */
+ err = ubi_attach_mtd_dev(p-name, p-vid_hdr_offs,
+p-data_offs);
+ if (err)
+ goto out_detach;
+ }
+
+ return 0;
+
+out_detach:
+ for (k = 0; k i; k++)
+ ubi_destroy_dev(k);
+ ubi_uif_global_close();
+out_alloc:
+ ubi_alloc_close();
+out_dbg:
+ ubi_dbg_close();
+ return err;
+}
+module_init(ubi_init);
+
+static void __exit ubi_exit(void)
+{
+ int i;
+
+ for (i = 0; i ubis_num; i++)
+ ubi_destroy_dev(i);
+ ubi_uif_global_close();
+ ubi_alloc_close();
+ ubi_dbg_close();
+}
+module_exit(ubi_exit);
+
+/**
+ * ubi_attach_mtd_dev - attach an MTD device.
+ *
+ * @mtd_dev: MTD device name or number string to attach
+ * @vid_hdr_offset: volume identifier header offset in physical eraseblocks
+ * @data_offset: data offset in physical eraseblock
+ *
+ * This function attaches an MTD device to UBI. It first treats @mtd_dev as the
+ * MTD device name, and tries to open it by this name. If it is unable to open,
+ * it tries to convert @mtd_dev to an integer and open the MTD device by its
+
