| Message ID | 1469440019-29358-2-git-send-email-rajeev_kumar@mentor.com |
|---|---|
| State | Rejected |
| Delegated to: | Brian Norris |
| Headers | show |
Rajeev, Am 25.07.2016 um 11:46 schrieb Rajeev Kumar: > If the master mtd does not have any slave mtd partitions, > and its numeraseregions is one(only has one erease block), and > we attach the master mtd with : ubiattach -m 0 -d 0 > > We will meet the error: > ------------------------------------------------------- > root ~$ ubiattach /dev/ubi_ctrl -m 0 -d 0 > UBI: attaching mtd0 to ubi0 > UBI error: io_init: multiple regions, not implemented > ubiattach: error!: cannot attach mtd0 > error 22 (Invalid argument) > ------------------------------------------------------- > > In fact, if there is only one "erase block", we should not > prevent the attach. > > This patch is tested against 3.14 kernel and only build test is > performed against current upstream master branch. The more interesting question is, why is ->numeraseregions not 0? The comment in the header says: /* Data for variable erase regions. If numeraseregions is zero, * it means that the whole device has erasesize as given above. */ So, if your MTD erase regions with the same size, it should be 0. IIRC we had such a discussion already on linux-mtd and it was not clear whether numeraseregions of 0 and 1 are equal or not. Thanks, //richard
Richard On 07/25/2016 04:01 PM, Richard Weinberger wrote: > Rajeev, > > Am 25.07.2016 um 11:46 schrieb Rajeev Kumar: >> If the master mtd does not have any slave mtd partitions, >> and its numeraseregions is one(only has one erease block), and >> we attach the master mtd with : ubiattach -m 0 -d 0 >> >> We will meet the error: >> ------------------------------------------------------- >> root ~$ ubiattach /dev/ubi_ctrl -m 0 -d 0 >> UBI: attaching mtd0 to ubi0 >> UBI error: io_init: multiple regions, not implemented >> ubiattach: error!: cannot attach mtd0 >> error 22 (Invalid argument) >> ------------------------------------------------------- >> >> In fact, if there is only one "erase block", we should not >> prevent the attach. >> >> This patch is tested against 3.14 kernel and only build test is >> performed against current upstream master branch. > > The more interesting question is, why is ->numeraseregions not 0? > > The comment in the header says: > /* Data for variable erase regions. If numeraseregions is zero, > * it means that the whole device has erasesize as given above. > */ > > So, if your MTD erase regions with the same size, it should be 0. > > IIRC we had such a discussion already on linux-mtd and it was not clear > whether numeraseregions of 0 and 1 are equal or not. > Could you please pass the link Thanks in advance ~Rajeev > Thanks, > //richard >
Rajeev, Am 25.07.2016 um 13:16 schrieb Rajeev Kumar: >> So, if your MTD erase regions with the same size, it should be 0. >> >> IIRC we had such a discussion already on linux-mtd and it was not clear >> whether numeraseregions of 0 and 1 are equal or not. >> > > Could you please pass the link > Thanks in advance http://lmgtfy.com/?q=numeraseregions+ubi+linux-mtd :-) *scnr*, //richard
(Artem, any opinions, since you had the most opinion last time this came up?) On Mon, Jul 25, 2016 at 12:31:39PM +0200, Richard Weinberger wrote: > Am 25.07.2016 um 11:46 schrieb Rajeev Kumar: > > If the master mtd does not have any slave mtd partitions, > > and its numeraseregions is one(only has one erease block), and > > we attach the master mtd with : ubiattach -m 0 -d 0 > > > > We will meet the error: > > ------------------------------------------------------- > > root ~$ ubiattach /dev/ubi_ctrl -m 0 -d 0 > > UBI: attaching mtd0 to ubi0 > > UBI error: io_init: multiple regions, not implemented > > ubiattach: error!: cannot attach mtd0 > > error 22 (Invalid argument) > > ------------------------------------------------------- > > > > In fact, if there is only one "erase block", we should not > > prevent the attach. > > > > This patch is tested against 3.14 kernel and only build test is > > performed against current upstream master branch. > > The more interesting question is, why is ->numeraseregions not 0? > > The comment in the header says: > /* Data for variable erase regions. If numeraseregions is zero, > * it means that the whole device has erasesize as given above. > */ > > So, if your MTD erase regions with the same size, it should be 0. > > IIRC we had such a discussion already on linux-mtd and it was not clear > whether numeraseregions of 0 and 1 are equal or not. I think 0 and 1 are essentially equal. But there's some potential room for error (e.g., if the driver doesn't configure mtd->erasesize == mtd->eraseregions[0].erasesize, and similar). Also, I see some problematic code like this in cfi_cmdset_0001.c: mtd->numeraseregions = cfi->cfiq->NumEraseRegions * cfi->numchips; So, if there are two chips, but both have a single erase region, with the same erasesize, we'll still end up with ->numeraseregions == 2. We can't hack all MTD users to start searching the eraseregions[] array to see if the device is actually uniform, even if it reports numeraseregions > 0. I'm inclined, then, to outlaw numeraseregions == 1 (to make it simpler for MTD users to handle), and put code either in drivers or in mtdcore.c to be slightly more intelligent (e.g., if the driver left numeraseregions == 1, then just do some sanity checking and re-set numeraseregions to 0). It might be good to move code like this from cfi_cmdset_0001.c into mtdcore.c at the same time too: if (offset != devsize) { /* Argh */ printk(KERN_WARNING "Sum of regions (%lx) != total size of set of interleaved chips (%lx)\n", offset, devsize); goto setup_err; } BTW, Rajeev, what devices are you testing? Just curious. Regards, Brian
On Fri, 2016-07-29 at 11:24 -0700, Brian Norris wrote: > (Artem, any opinions, since you had the most opinion last time this > came > up?) Hi Brian, well, I do not have strong opinion. That UBI translates to "I do not know how to deal with multiple regions", nothing else. I think this was my understanding of numeraseregions 1. numaraseregions=0: no regions, just uniform flash. 2. numeraseregions=1: basically same as above, but friendlier to the region-aware software. Indeed, if I care about regions, I do not want to handle the special case of numerasereions=0, I want a common case with numerasereionts>0. Or to put it this way, from the drivers' writer POW: numaraseregions=0 - do not know anything about regions, do not care. Also pre-regions dirivers, old stuff numeraseregions=1 - I want my driver to be ideal. Non-aware SW won't look to the regions stuff, the aware SW will work nicely. So my take is: ban numeraseregions=1 if you feel like, but I recommend to just document the 0 (don't care/legacy) and 1 (same as 0, but care) and allow for both. Artem.
Hello Brian, Artem, Are you suggesting that this change be documented instead. Note that we are using NOR flash devices for our testing as you asked. Can I request you to please clarify on acceptance of this patch ? Thanks Sanjeev >-----Original Message----- >From: Artem Bityutskiy [mailto:dedekind1@gmail.com] >Sent: Thursday, August 04, 2016 12:01 PM >To: Brian Norris; Richard Weinberger >Cc: Rajeev Kumar; dwmw2@infradead.org; linux-mtd@lists.infradead.org; >stable@vger.kernel.org >Subject: Re: [PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd- >>numeraseregions is 1 > >On Fri, 2016-07-29 at 11:24 -0700, Brian Norris wrote: >> (Artem, any opinions, since you had the most opinion last time this >> came >> up?) > >Hi Brian, > >well, I do not have strong opinion. That UBI translates to "I do not know how to >deal with multiple regions", nothing else. > >I think this was my understanding of numeraseregions > >1. numaraseregions=0: no regions, just uniform flash. > >2. numeraseregions=1: basically same as above, but friendlier to the region- >aware software. Indeed, if I care about regions, I do not want to handle the >special case of numerasereions=0, I want a common case with >numerasereionts>0. > >Or to put it this way, from the drivers' writer POW: > >numaraseregions=0 - do not know anything about regions, do not care. >Also pre-regions dirivers, old stuff > >numeraseregions=1 - I want my driver to be ideal. Non-aware SW won't look to >the regions stuff, the aware SW will work nicely. > >So my take is: ban numeraseregions=1 if you feel like, but I recommend to just >document the 0 (don't care/legacy) and 1 (same as 0, but care) and allow for >both. > >Artem.
Just to clarify, I meant since you were suggesting that value of numeraseregions either 0 or 1 is essentially equal. Are you suggesting that you don't like the change and better clarify this in documentation about this ? -Sanjeev >-----Original Message----- >From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of >Chugh, Sanjeev >Sent: Friday, September 23, 2016 4:51 PM >To: dedekind1@gmail.com; Brian Norris; Richard Weinberger >Cc: linux-mtd@lists.infradead.org; dwmw2@infradead.org; >stable@vger.kernel.org >Subject: RE: [PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd- >>numeraseregions is 1 > >Hello Brian, Artem, > >Are you suggesting that this change be documented instead. Note that we are >using NOR flash devices for our testing as you asked. > >Can I request you to please clarify on acceptance of this patch ? > >Thanks >Sanjeev > >>-----Original Message----- >>From: Artem Bityutskiy [mailto:dedekind1@gmail.com] >>Sent: Thursday, August 04, 2016 12:01 PM >>To: Brian Norris; Richard Weinberger >>Cc: Rajeev Kumar; dwmw2@infradead.org; linux-mtd@lists.infradead.org; >>stable@vger.kernel.org >>Subject: Re: [PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd- >>>numeraseregions is 1 >> >>On Fri, 2016-07-29 at 11:24 -0700, Brian Norris wrote: >>> (Artem, any opinions, since you had the most opinion last time this >>> came >>> up?) >> >>Hi Brian, >> >>well, I do not have strong opinion. That UBI translates to "I do not know how to >>deal with multiple regions", nothing else. >> >>I think this was my understanding of numeraseregions >> >>1. numaraseregions=0: no regions, just uniform flash. >> >>2. numeraseregions=1: basically same as above, but friendlier to the region- >>aware software. Indeed, if I care about regions, I do not want to handle the >>special case of numerasereions=0, I want a common case with >>numerasereionts>0. >> >>Or to put it this way, from the drivers' writer POW: >> >>numaraseregions=0 - do not know anything about regions, do not care. >>Also pre-regions dirivers, old stuff >> >>numeraseregions=1 - I want my driver to be ideal. Non-aware SW won't look to >>the regions stuff, the aware SW will work nicely. >> >>So my take is: ban numeraseregions=1 if you feel like, but I recommend to just >>document the 0 (don't care/legacy) and 1 (same as 0, but care) and allow for >>both. >> >>Artem. >______________________________________________________ >Linux MTD discussion mailing list >http://lists.infradead.org/mailman/listinfo/linux-mtd/
>-----Original Message----- >From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of >Chugh, Sanjeev >Sent: Friday, September 23, 2016 4:51 PM >To: dedekind1@gmail.com; Brian Norris; Richard Weinberger >Cc: linux-mtd@lists.infradead.org; dwmw2@infradead.org; >stable@vger.kernel.org >Subject: RE: [PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd- >>numeraseregions is 1 > >Hello Brian, Artem, > >Are you suggesting that this change be documented instead. Note that we are >using NOR flash devices for our testing as you asked. > >Can I request you to please clarify on acceptance of this patch ? > >Thanks >Sanjeev > >>-----Original Message----- >>From: Artem Bityutskiy [mailto:dedekind1@gmail.com] >>Sent: Thursday, August 04, 2016 12:01 PM >>To: Brian Norris; Richard Weinberger >>Cc: Rajeev Kumar; dwmw2@infradead.org; linux-mtd@lists.infradead.org; >>stable@vger.kernel.org >>Subject: Re: [PATCH 2/2] ubi: attach: do not return -EINVAL if the mtd- >>>numeraseregions is 1 >> >>On Fri, 2016-07-29 at 11:24 -0700, Brian Norris wrote: >>> (Artem, any opinions, since you had the most opinion last time this >>> came >>> up?) >> >>Hi Brian, >> >>well, I do not have strong opinion. That UBI translates to "I do not know how to >>deal with multiple regions", nothing else. >> >>I think this was my understanding of numeraseregions >> >>1. numaraseregions=0: no regions, just uniform flash. >> >>2. numeraseregions=1: basically same as above, but friendlier to the region- >>aware software. Indeed, if I care about regions, I do not want to handle the >>special case of numerasereions=0, I want a common case with >>numerasereionts>0. >> >>Or to put it this way, from the drivers' writer POW: >> >>numaraseregions=0 - do not know anything about regions, do not care. >>Also pre-regions dirivers, old stuff >> >>numeraseregions=1 - I want my driver to be ideal. Non-aware SW won't look to >>the regions stuff, the aware SW will work nicely. >> >>So my take is: ban numeraseregions=1 if you feel like, but I recommend to just >>document the 0 (don't care/legacy) and 1 (same as 0, but care) and allow for >>both. >> >>Artem. >______________________________________________________ >Linux MTD discussion mailing list >http://lists.infradead.org/mailman/listinfo/linux-mtd/ Hello Brian, Artem, etc. al. I would really appreciate if you clarify what's your opinion on the patch. I would like to close the thread if it's not going to be accepted. Or I would try making further changes but I need inputs. Thanks in advance. -Sanjeev
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c index 59229c4..562421b 100644 --- a/drivers/mtd/ubi/build.c +++ b/drivers/mtd/ubi/build.c @@ -628,7 +628,7 @@ static int io_init(struct ubi_device *ubi, int max_beb_per1024) dbg_gen("sizeof(struct ubi_ainf_peb) %zu", sizeof(struct ubi_ainf_peb)); dbg_gen("sizeof(struct ubi_wl_entry) %zu", sizeof(struct ubi_wl_entry)); - if (ubi->mtd->numeraseregions != 0) { + if (ubi->mtd->numeraseregions > 1) { /* * Some flashes have several erase regions. Different regions * may have different eraseblock size and other
If the master mtd does not have any slave mtd partitions, and its numeraseregions is one(only has one erease block), and we attach the master mtd with : ubiattach -m 0 -d 0 We will meet the error: ------------------------------------------------------- root ~$ ubiattach /dev/ubi_ctrl -m 0 -d 0 UBI: attaching mtd0 to ubi0 UBI error: io_init: multiple regions, not implemented ubiattach: error!: cannot attach mtd0 error 22 (Invalid argument) ------------------------------------------------------- In fact, if there is only one "erase block", we should not prevent the attach. This patch is tested against 3.14 kernel and only build test is performed against current upstream master branch. Signed-off-by: Rajeev Kumar <rajeev_kumar@mentor.com> --- drivers/mtd/ubi/build.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)