Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Andrew Morton]

On Thu, 21 Aug 2008 19:00:55 -0700 (GMT-07:00)
Bruce Leonard <brucle@earthlink.net> wrote:

> --- a/include/mtd/mtd-abi.h
> +++ b/include/mtd/mtd-abi.h
> @@ -6,7 +6,7 @@
>  #define __MTD_ABI_H__
>  
>  struct erase_info_user {
> -	uint32_t start;
> +	uint64_t start;
>  	uint32_t length;
>  };
>  
> @@ -50,7 +50,7 @@ struct mtd_oob_buf {
>  struct mtd_info_user {
>  	uint8_t type;
>  	uint32_t flags;
> -	uint32_t size;	 // Total size of the MTD
> +	uint64_t size;	 // Total size of the MTD
>  	uint32_t erasesize;
>  	uint32_t writesize;
>  	uint32_t oobsize;   // Amount of OOB data per block (e.g. 16)

This changes the kernel<->userspace ABI and is hence a big no-no.  I
assume that this change will cause old userspace to malfunction on new
kernels, and vice versa.

Supporting >2Gb MTD devices sounds useful (I'm surprised that we don't
already do so).

Please cc linux-mtd@lists.infradead.org (at least) on MTD-related
patches, thanks.

Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Bruce_Leonard]

linux-mtd-bounces@lists.infradead.org wrote on 08/26/2008 04:55:36 PM:

> On Thu, 21 Aug 2008 19:00:55 -0700 (GMT-07:00)
> Bruce Leonard <brucle@earthlink.net> wrote:
> 
> > --- a/include/mtd/mtd-abi.h
> > +++ b/include/mtd/mtd-abi.h
> > @@ -6,7 +6,7 @@
> >  #define __MTD_ABI_H__
> > 
> >  struct erase_info_user {
> > -   uint32_t start;
> > +   uint64_t start;
> >     uint32_t length;
> >  };
> > 
> > @@ -50,7 +50,7 @@ struct mtd_oob_buf {
> >  struct mtd_info_user {
> >     uint8_t type;
> >     uint32_t flags;
> > -   uint32_t size;    // Total size of the MTD
> > +   uint64_t size;    // Total size of the MTD
> >     uint32_t erasesize;
> >     uint32_t writesize;
> >     uint32_t oobsize;   // Amount of OOB data per block (e.g. 16)
> 
> This changes the kernel<->userspace ABI and is hence a big no-no.  I
> assume that this change will cause old userspace to malfunction on new
> kernels, and vice versa.
> 

Well, in my posting I noted that the mtd-utils were broken because of this 
but I didn't really have any idea as to how to fix things.  I can see why 
it would be a big no-no to change this.  Do you have any suggestions on 
what I could do differently to prevent making that change?

> Supporting >2Gb MTD devices sounds useful (I'm surprised that we don't
> already do so).
> 

There was a LOT of interest in this over the last few months while I was 
working on it, but a very suprising silence has developed since I posted 
the patches.  I guess I'm more cutting edge than I thought :).

> Please cc linux-mtd@lists.infradead.org (at least) on MTD-related
> patches, thanks.

I started with the MTD list and then also posted to lkml when I realized I 
had forgotten to CC it.

Thanks.

Bruce

RE: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Tim Anderson]

Bruce,

Since you don't want to change the ABI you need to extend it. Andrew, should
he add an new ioctl that passes a new structure definition? That way the
original code works and you have to do a new ioctl to get the new size data.

> -----Original Message-----
> From: linux-mtd-bounces@lists.infradead.org 
> [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of 
> Bruce_Leonard@selinc.com
> Sent: Tuesday, August 26, 2008 6:21 PM
> To: Andrew Morton
> Cc: linux-mtd-bounces@lists.infradead.org; 
> linux-mtd@lists.infradead.org; David Woodhouse; 
> linux-kernel@vger.kernel.org; Bruce Leonard
> Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices
> 
> linux-mtd-bounces@lists.infradead.org wrote on 08/26/2008 04:55:36 PM:
> 
> > On Thu, 21 Aug 2008 19:00:55 -0700 (GMT-07:00)
> > Bruce Leonard <brucle@earthlink.net> wrote:
> > 
> > > --- a/include/mtd/mtd-abi.h
> > > +++ b/include/mtd/mtd-abi.h
> > > @@ -6,7 +6,7 @@
> > >  #define __MTD_ABI_H__
> > > 
> > >  struct erase_info_user {
> > > -   uint32_t start;
> > > +   uint64_t start;
> > >     uint32_t length;
> > >  };
> > > 
> > > @@ -50,7 +50,7 @@ struct mtd_oob_buf {
> > >  struct mtd_info_user {
> > >     uint8_t type;
> > >     uint32_t flags;
> > > -   uint32_t size;    // Total size of the MTD
> > > +   uint64_t size;    // Total size of the MTD
> > >     uint32_t erasesize;
> > >     uint32_t writesize;
> > >     uint32_t oobsize;   // Amount of OOB data per block (e.g. 16)
> > 
> > This changes the kernel<->userspace ABI and is hence a big no-no.  I
> > assume that this change will cause old userspace to 
> malfunction on new
> > kernels, and vice versa.
> > 
> 
> Well, in my posting I noted that the mtd-utils were broken 
> because of this 
> but I didn't really have any idea as to how to fix things.  I 
> can see why 
> it would be a big no-no to change this.  Do you have any 
> suggestions on 
> what I could do differently to prevent making that change?
> 
> > Supporting >2Gb MTD devices sounds useful (I'm surprised 
> that we don't
> > already do so).
> > 
> 
> There was a LOT of interest in this over the last few months 
> while I was 
> working on it, but a very suprising silence has developed 
> since I posted 
> the patches.  I guess I'm more cutting edge than I thought :).
> 
> > Please cc linux-mtd@lists.infradead.org (at least) on MTD-related
> > patches, thanks.
> 
> I started with the MTD list and then also posted to lkml when 
> I realized I 
> had forgotten to CC it.
> 
> Thanks.
> 
> Bruce
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 

Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Andrew Morton]

On Tue, 26 Aug 2008 18:40:26 -0700 "Tim Anderson" <tanderson@mvista.com> wrote:

> > -----Original Message-----
> > From: linux-mtd-bounces@lists.infradead.org 
> > [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of 
> > Bruce_Leonard@selinc.com
> > Sent: Tuesday, August 26, 2008 6:21 PM
> > To: Andrew Morton
> > Cc: linux-mtd-bounces@lists.infradead.org; 
> > linux-mtd@lists.infradead.org; David Woodhouse; 
> > linux-kernel@vger.kernel.org; Bruce Leonard
> > Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices
> > 
> > linux-mtd-bounces@lists.infradead.org wrote on 08/26/2008 04:55:36 PM:
> > 
> > > On Thu, 21 Aug 2008 19:00:55 -0700 (GMT-07:00)
> > > Bruce Leonard <brucle@earthlink.net> wrote:
> > > 
> > > > --- a/include/mtd/mtd-abi.h
> > > > +++ b/include/mtd/mtd-abi.h
> > > > @@ -6,7 +6,7 @@
> > > >  #define __MTD_ABI_H__
> > > > 
> > > >  struct erase_info_user {
> > > > -   uint32_t start;
> > > > +   uint64_t start;
> > > >     uint32_t length;
> > > >  };
> > > > 
> > > > @@ -50,7 +50,7 @@ struct mtd_oob_buf {
> > > >  struct mtd_info_user {
> > > >     uint8_t type;
> > > >     uint32_t flags;
> > > > -   uint32_t size;    // Total size of the MTD
> > > > +   uint64_t size;    // Total size of the MTD
> > > >     uint32_t erasesize;
> > > >     uint32_t writesize;
> > > >     uint32_t oobsize;   // Amount of OOB data per block (e.g. 16)
> > > 
> > > This changes the kernel<->userspace ABI and is hence a big no-no.  I
> > > assume that this change will cause old userspace to 
> > malfunction on new
> > > kernels, and vice versa.
> > > 
> > 
> > Well, in my posting I noted that the mtd-utils were broken 
> > because of this 
> > but I didn't really have any idea as to how to fix things.  I 
> > can see why 
> > it would be a big no-no to change this.  Do you have any 
> > suggestions on 
> > what I could do differently to prevent making that change?
> > 
> > > Supporting >2Gb MTD devices sounds useful (I'm surprised 
> > that we don't
> > > already do so).
> > > 
> > 
> > There was a LOT of interest in this over the last few months 
> > while I was 
> > working on it, but a very suprising silence has developed 
> > since I posted 
> > the patches.  I guess I'm more cutting edge than I thought :).
> > 

(top-posting repaired.  Please don't.)

> Bruce,
> 
> Since you don't want to change the ABI you need to extend it. Andrew, should
> he add an new ioctl that passes a new structure definition? That way the
> original code works and you have to do a new ioctl to get the new size data.
> 

Yes, I expect that a new ioctl would be best.  Sometimes it is possible
to extend an existing one (and its associated payload) but that
requires versioning info which usually wasn't thought of on day one.

But David's the man.

RE: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Bruce_Leonard]

Now there's something I hadn't thought of.  Thanks, I'll look into doing 
it that way.


"Tim Anderson" <tanderson@mvista.com> wrote on 08/26/2008 06:40:26 PM:

> Bruce,
> 
> Since you don't want to change the ABI you need to extend it. Andrew, 
should
> he add an new ioctl that passes a new structure definition? That way the
> original code works and you have to do a new ioctl to get the new size 
data.
> 
> > -----Original Message-----
> > From: linux-mtd-bounces@lists.infradead.org 
> > [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of 
> > Bruce_Leonard@selinc.com
> > Sent: Tuesday, August 26, 2008 6:21 PM
> > To: Andrew Morton
> > Cc: linux-mtd-bounces@lists.infradead.org; 
> > linux-mtd@lists.infradead.org; David Woodhouse; 
> > linux-kernel@vger.kernel.org; Bruce Leonard
> > Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices
> > 
> > linux-mtd-bounces@lists.infradead.org wrote on 08/26/2008 04:55:36 PM:
> > 
> > > On Thu, 21 Aug 2008 19:00:55 -0700 (GMT-07:00)
> > > Bruce Leonard <brucle@earthlink.net> wrote:
> > > 
> > > > --- a/include/mtd/mtd-abi.h
> > > > +++ b/include/mtd/mtd-abi.h
> > > > @@ -6,7 +6,7 @@
> > > >  #define __MTD_ABI_H__
> > > > 
> > > >  struct erase_info_user {
> > > > -   uint32_t start;
> > > > +   uint64_t start;
> > > >     uint32_t length;
> > > >  };
> > > > 
> > > > @@ -50,7 +50,7 @@ struct mtd_oob_buf {
> > > >  struct mtd_info_user {
> > > >     uint8_t type;
> > > >     uint32_t flags;
> > > > -   uint32_t size;    // Total size of the MTD
> > > > +   uint64_t size;    // Total size of the MTD
> > > >     uint32_t erasesize;
> > > >     uint32_t writesize;
> > > >     uint32_t oobsize;   // Amount of OOB data per block (e.g. 16)
> > > 
> > > This changes the kernel<->userspace ABI and is hence a big no-no.  I
> > > assume that this change will cause old userspace to 
> > malfunction on new
> > > kernels, and vice versa.
> > > 
> > 
> > Well, in my posting I noted that the mtd-utils were broken 
> > because of this 
> > but I didn't really have any idea as to how to fix things.  I 
> > can see why 
> > it would be a big no-no to change this.  Do you have any 
> > suggestions on 
> > what I could do differently to prevent making that change?
> > 
> > > Supporting >2Gb MTD devices sounds useful (I'm surprised 
> > that we don't
> > > already do so).
> > > 
> > 
> > There was a LOT of interest in this over the last few months 
> > while I was 
> > working on it, but a very suprising silence has developed 
> > since I posted 
> > the patches.  I guess I'm more cutting edge than I thought :).
> > 
> > > Please cc linux-mtd@lists.infradead.org (at least) on MTD-related
> > > patches, thanks.
> > 
> > I started with the MTD list and then also posted to lkml when 
> > I realized I 
> > had forgotten to CC it.
> > 
> > Thanks.
> > 
> > Bruce
> > 
> > ______________________________________________________
> > Linux MTD discussion mailing list
> > http://lists.infradead.org/mailman/listinfo/linux-mtd/
> > 
> 

Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Artem Bityutskiy]

Hi Bruce,

On Tue, 2008-08-26 at 18:21 -0700, Bruce_Leonard@selinc.com wrote:
> > This changes the kernel<->userspace ABI and is hence a big no-no.  I
> > assume that this change will cause old userspace to malfunction on new
> > kernels, and vice versa.
> > 
> 
> Well, in my posting I noted that the mtd-utils were broken because of this 
> but I didn't really have any idea as to how to fix things.  I can see why 
> it would be a big no-no to change this.  Do you have any suggestions on 
> what I could do differently to prevent making that change?

Unfortunately, it is unacceptable to break existing stuff, even old
crappy mtd utils :-). That's simply how we work in the kernel community.

Stuff which you do is always not simple because you have to make sure
all legacy stuff does not break. So you'll need to invest more time into
your work.

> There was a LOT of interest in this over the last few months while I was 
> working on it, but a very suprising silence has developed since I posted 
> the patches.  I guess I'm more cutting edge than I thought :).

I understand your frustration. Apologies for not reacting faster.

> > Please cc linux-mtd@lists.infradead.org (at least) on MTD-related
> > patches, thanks.
> 
> I started with the MTD list and then also posted to lkml when I realized I 
> had forgotten to CC it.

Yeah, sometimes MTD ML just ignores patches, I am in the same boat, but
even if you've decided to send stuff to lkml, please, CC MTD ML anyway.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

RE: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Tim Anderson]

Bruce,

I have been experimenting with this. I think there is at least 3 maybe 4
IOCTLS that are going to need to have a LARGE mode. The 2 most obvious are
MEMGETINFO and MEMERASE. I was starting to code something up that passes the
size as blocks as opposed to bytes. That may be more consistent with UBI. I am
doing a little experimenting if you want some help there. In any case, it can
be done where the old utility can work (for 2GB and less size devices) and a
new call not supported by the kernel will let the utility try the new
interface and revert to the old if the kernel does not support block mode
interfaces.

Then we can either re-use the existing structures passed through another
ioctl, changing the definition of START and SIZE to be blocks or define new
erase_info_user and mtd_info_user structures for the large call.

David, would you thing re-using the same structure would be to obscure?

I have started looking at a MEMGETLINFO defining size as blocks instead of
bytes.


> -----Original Message-----
> From: linux-mtd-bounces@lists.infradead.org 
> [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of 
> Bruce_Leonard@selinc.com
> Sent: Tuesday, August 26, 2008 6:21 PM
> To: Andrew Morton
> Cc: linux-mtd-bounces@lists.infradead.org; 
> linux-mtd@lists.infradead.org; David Woodhouse; 
> linux-kernel@vger.kernel.org; Bruce Leonard
> Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices
> 
> linux-mtd-bounces@lists.infradead.org wrote on 08/26/2008 04:55:36 PM:
> 
> > On Thu, 21 Aug 2008 19:00:55 -0700 (GMT-07:00)
> > Bruce Leonard <brucle@earthlink.net> wrote:
> > 
> > > --- a/include/mtd/mtd-abi.h
> > > +++ b/include/mtd/mtd-abi.h
> > > @@ -6,7 +6,7 @@
> > >  #define __MTD_ABI_H__
> > > 
> > >  struct erase_info_user {
> > > -   uint32_t start;
> > > +   uint64_t start;
> > >     uint32_t length;
> > >  };
> > > 
> > > @@ -50,7 +50,7 @@ struct mtd_oob_buf {
> > >  struct mtd_info_user {
> > >     uint8_t type;
> > >     uint32_t flags;
> > > -   uint32_t size;    // Total size of the MTD
> > > +   uint64_t size;    // Total size of the MTD
> > >     uint32_t erasesize;
> > >     uint32_t writesize;
> > >     uint32_t oobsize;   // Amount of OOB data per block (e.g. 16)
> > 
> > This changes the kernel<->userspace ABI and is hence a big no-no.  I
> > assume that this change will cause old userspace to 
> malfunction on new
> > kernels, and vice versa.
> > 
> 
> Well, in my posting I noted that the mtd-utils were broken 
> because of this 
> but I didn't really have any idea as to how to fix things.  I 
> can see why 
> it would be a big no-no to change this.  Do you have any 
> suggestions on 
> what I could do differently to prevent making that change?
> 
> > Supporting >2Gb MTD devices sounds useful (I'm surprised 
> that we don't
> > already do so).
> > 
> 
> There was a LOT of interest in this over the last few months 
> while I was 
> working on it, but a very suprising silence has developed 
> since I posted 
> the patches.  I guess I'm more cutting edge than I thought :).
> 
> > Please cc linux-mtd@lists.infradead.org (at least) on MTD-related
> > patches, thanks.
> 
> I started with the MTD list and then also posted to lkml when 
> I realized I 
> had forgotten to CC it.
> 
> Thanks.
> 
> Bruce
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 

RE: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Artem Bityutskiy]

On Tue, 2008-08-26 at 23:01 -0700, Tim Anderson wrote:
> I have been experimenting with this. I think there is at least 3 maybe 4
> IOCTLS that are going to need to have a LARGE mode. The 2 most obvious are
> MEMGETINFO and MEMERASE.

dwmw2 vetoed any MEMGETINFO ioctl changes, or new similar ioctls, and it
kind of make sense, because we should use sysfs instead. But this, in
turn, means implementing sysfs support for MTD, because MTD is not
LinuxDeviceModel-enabled at the moment.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

RE: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Tim Anderson]

Artem,

I see your point. Ioctls are going away after all.

So we should have something like:
/sys/class/mtd/mtd0/

Or just under device or platform? 

> -----Original Message-----
> From: Artem Bityutskiy [mailto:dedekind@infradead.org] 
> Sent: Tuesday, August 26, 2008 11:04 PM
> To: Tim Anderson
> Cc: Bruce_Leonard@selinc.com; 'Andrew Morton'; 
> linux-mtd-bounces@lists.infradead.org; 'David Woodhouse'; 
> linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.org; 
> 'Bruce Leonard'
> Subject: RE: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices
> 
> On Tue, 2008-08-26 at 23:01 -0700, Tim Anderson wrote:
> > I have been experimenting with this. I think there is at 
> least 3 maybe 4
> > IOCTLS that are going to need to have a LARGE mode. The 2 
> most obvious are
> > MEMGETINFO and MEMERASE.
> 
> dwmw2 vetoed any MEMGETINFO ioctl changes, or new similar 
> ioctls, and it
> kind of make sense, because we should use sysfs instead. But this, in
> turn, means implementing sysfs support for MTD, because MTD is not
> LinuxDeviceModel-enabled at the moment.
> 
> -- 
> Best regards,
> Artem Bityutskiy (???????? ?????)
> 

RE: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[David Woodhouse]

On Tue, 2008-08-26 at 23:20 -0700, Tim Anderson wrote:
> I see your point. Ioctls are going away after all.
> 
> So we should have something like:
> /sys/class/mtd/mtd0/
> 
> Or just under device or platform? 

Tim, please stop top-posting.

I'm not sure I'd say I _vetoed_ new ioctls, but I certainly expressed a
desire to do all the information stuff through sysfs. We would need a
new MEMERASE64 ioctl though.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

RE: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Ricard Wanderlof]

On Wed, 27 Aug 2008, David Woodhouse wrote:

> On Tue, 2008-08-26 at 23:20 -0700, Tim Anderson wrote:
>> I see your point. Ioctls are going away after all.
>>
>> So we should have something like:
>> /sys/class/mtd/mtd0/
>>
>> Or just under device or platform?
>
> Tim, please stop top-posting.
>
> I'm not sure I'd say I _vetoed_ new ioctls, but I certainly expressed a
> desire to do all the information stuff through sysfs. We would need a
> new MEMERASE64 ioctl though.

If the (lack of) sysfs stuff is standing between us and a nice >4GiB mtd I 
for one would think that a set of 64-bit ioctls would be a reasonable 
halfway house.

/Ricard
--
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Artem Bityutskiy]

Ricard Wanderlof wrote:
>> I'm not sure I'd say I _vetoed_ new ioctls, but I certainly expressed a
>> desire to do all the information stuff through sysfs. We would need a
>> new MEMERASE64 ioctl though.
> 
> If the (lack of) sysfs stuff is standing between us and a nice >4GiB mtd 
> I for one would think that a set of 64-bit ioctls would be a reasonable 
> halfway house.

On the other hand we'll stay with half-solutions forever if we allow them.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Bruce Leonard]

On Aug 26, 2008, at 11:49 PM, Ricard Wanderlof wrote:
>
> If the (lack of) sysfs stuff is standing between us and a nice >4GiB  
> mtd I for one would think that a set of 64-bit ioctls would be a  
> reasonable halfway house.
>

Having started this mess, I'll throw my half euros worth in :).  While  
I agree that sysfs is the right way to go, I'm not going to be able to  
pull it off.  Not withstanding my total lack of knowledge about sysfs,  
I've been very fortunate in that I've been doing all of this work on  
company time.  Due to project constraints, that company time is coming  
to an end.  I could eek out another week or two and get a set of 64- 
bit ioctls, but I will not be able to spend a couple more months of  
company time learning sysfs and implementing it.  And I just don't  
have any personal time to spare right now.  I'm sorry, if it's  
anything other than ioctl I just can't do it.

That being said, if a set of ioctls is acceptable, I'm more than happy  
to take that on as soon as we reach a consensus.

Bruce

RE: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Artem Bityutskiy]

On Tue, 2008-08-26 at 23:20 -0700, Tim Anderson wrote:
> Artem,
> 
> I see your point. Ioctls are going away after all.
> 
> So we should have something like:
> /sys/class/mtd/mtd0/
> 
> Or just under device or platform? 

I am not very good in LDM, but it does not look like MTD stuff should go
to /sys/class/. I'd say it should be in /sys/devices/.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Alan Cox]

On Tue, 26 Aug 2008 23:20:46 -0700
"Tim Anderson" <tanderson@mvista.com> wrote:

> Artem,
> 
> I see your point. Ioctls are going away after all.

I don't know where that stupid story keeps coming from. Ioctl is alive
and well and there are more not less of them. There are lots of things
you *cannot* do with sysfs, including synchronization and handling
many kinds of changes to objects that can appear and disappear. Ditto
there are problems with getting a consistent snapshot via sysfs because
you can't atomically read multiple fields.

So please stop this 'ioctls are going away' stuff, its bunkum.

Alan

Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[David Woodhouse]

On Wed, 2008-08-27 at 09:39 +0100, Alan Cox wrote:
> On Tue, 26 Aug 2008 23:20:46 -0700
> "Tim Anderson" <tanderson@mvista.com> wrote:
> 
> > Artem,
> > 
> > I see your point. Ioctls are going away after all.
> 
> I don't know where that stupid story keeps coming from. Ioctl is alive
> and well and there are more not less of them. There are lots of things
> you *cannot* do with sysfs, including synchronization and handling
> many kinds of changes to objects that can appear and disappear. Ditto
> there are problems with getting a consistent snapshot via sysfs because
> you can't atomically read multiple fields.
> 
> So please stop this 'ioctls are going away' stuff, its bunkum.

True, and we'll definitely need a new MEMERASE64 ioctl. But for the
_informational_ parts, those can happily be done through sysfs.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Jörn Engel]

On Wed, 27 August 2008 10:01:32 +0100, David Woodhouse wrote:
> 
> True, and we'll definitely need a new MEMERASE64 ioctl. But for the
> _informational_ parts, those can happily be done through sysfs.

They certainly can, but should they?  There may be some reason to prefer
sysfs that should be self-evident - except that I'm a bit thick and seem
to need it spelled out.  Or maybe I've just become disillusioned with
the practice of replacing crappy interfaces (ioctl here) with other
crappy interfaces (sysfs here) and having to support both for all
eternity.  sysctl, ioctl, proc, sysfs, debugfs, netlink, ... -
individually they all suck in their own peculiar way.  But together they
create a mess I no longer dare to name.

So what was the reason again why mtd needs two userspace interfaces
instead of just one?

Jörn

-- 
It does not require a majority to prevail, but rather an irate,
tireless minority keen to set brush fires in people's minds.
-- Samuel Adams

Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Artem Bityutskiy]

On Wed, 2008-08-27 at 16:34 +0200, Jörn Engel wrote:
> They certainly can, but should they?  There may be some reason to prefer
> sysfs that should be self-evident - except that I'm a bit thick and seem
> to need it spelled out.  Or maybe I've just become disillusioned with
> the practice of replacing crappy interfaces (ioctl here) with other
> crappy interfaces (sysfs here) and having to support both for all
> eternity.  sysctl, ioctl, proc, sysfs, debugfs, netlink, ... -
> individually they all suck in their own peculiar way.  But together they
> create a mess I no longer dare to name.

The plus of sysfs I see is that I can add more files to expose more
information in sysfs, while I can not change MEMGETINFO ioctl. E.g., I
need to expose sub-page size to user-space, and I cannot do this with
MEMGETINFO.

> So what was the reason again why mtd needs two userspace interfaces
> instead of just one?

I would like to make udev creating MTD devices, instead of creating them
by hands. Adding MTD to LDM would anyway introduce corresponding sysfs
files, right? This means we would have one more interface anyway.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Jörn Engel]

On Wed, 27 August 2008 17:47:43 +0300, Artem Bityutskiy wrote:
> 
> The plus of sysfs I see is that I can add more files to expose more
> information in sysfs, while I can not change MEMGETINFO ioctl. E.g., I
> need to expose sub-page size to user-space, and I cannot do this with
> MEMGETINFO.

sysfs makes adding new attributes easier, yes.  But once added you
cannot remove the attribute again - ever.  Which means that either way
you need to tread carefully and think twice before making a rash
decision.

> > So what was the reason again why mtd needs two userspace interfaces
> > instead of just one?
> 
> I would like to make udev creating MTD devices, instead of creating them
> by hands. Adding MTD to LDM would anyway introduce corresponding sysfs
> files, right? This means we would have one more interface anyway.

Could be useful, I don't mind you sending a patch.  However, does this
means that MEMGETINFO64 or some other ioctl should not be done?  Should
flash_erase open, read and close 8 seperate files instead of doing a
single ioctl?  And should our support for large devices wait for the
sysfs support that has been talked about and not done for about two
years already?

Call me a sceptic.

Jörn

-- 
The cheapest, fastest and most reliable components of a computer
system are those that aren't there.
-- Gordon Bell, DEC labratories

Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Artem Bityutskiy]

On Wed, 2008-08-27 at 17:25 +0200, Jörn Engel wrote:
> > I would like to make udev creating MTD devices, instead of creating them
> > by hands. Adding MTD to LDM would anyway introduce corresponding sysfs
> > files, right? This means we would have one more interface anyway.
> 
> Could be useful, I don't mind you sending a patch.  However, does this
> means that MEMGETINFO64 or some other ioctl should not be done?  Should
> flash_erase open, read and close 8 seperate files instead of doing a
> single ioctl?  And should our support for large devices wait for the
> sysfs support that has been talked about and not done for about two
> years already?

Up to dwmw2, but I do not mind if the answer to all the above questions
is "yes" :-)

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Jörn Engel]

On Fri, 29 August 2008 08:48:07 +0300, Artem Bityutskiy wrote:
> On Wed, 2008-08-27 at 17:25 +0200, Jörn Engel wrote:
> > 
> > Could be useful, I don't mind you sending a patch.  However, does this
> > means that MEMGETINFO64 or some other ioctl should not be done?  Should
> > flash_erase open, read and close 8 seperate files instead of doing a
> > single ioctl?  And should our support for large devices wait for the
> > sysfs support that has been talked about and not done for about two
> > years already?
> 
> Up to dwmw2, but I do not mind if the answer to all the above questions
> is "yes" :-)

Well, I personally think a "yes" to the last question would be utter
madness.  Whoever answers that should better come up with an alternative
patch for sysfs support pronto.

Large flashes are not a one-off cases where a single manufacturer had a
rather bizarre design.  Their numbers will continually increase.  And
not supporting an ever-increasing class of hardware is simply not an
option.

Jörn

-- 
 on a false concept. This concept is that
people actually want to look at source code.
-- Rob Enderle

Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[David Woodhouse]

On Wed, 2008-08-27 at 17:25 +0200, Jörn Engel wrote:
> On Wed, 27 August 2008 17:47:43 +0300, Artem Bityutskiy wrote:
> > 
> > The plus of sysfs I see is that I can add more files to expose more
> > information in sysfs, while I can not change MEMGETINFO ioctl. E.g., I
> > need to expose sub-page size to user-space, and I cannot do this with
> > MEMGETINFO.
> 
> sysfs makes adding new attributes easier, yes.  But once added you
> cannot remove the attribute again - ever.  Which means that either way
> you need to tread carefully and think twice before making a rash
> decision.

That's not _necessarily_ true, although it should certainly be done with
care. Attributes in sysfs can be optional (in a way that they can't
really be optional if they're part of a binary ioctl payload), and
userspace can cope with them being absent. The sub-page size attribute
is something which wouldn't always be present, and we could happily just
drop it and forget about it in future if we really wanted to.

> > > So what was the reason again why mtd needs two userspace interfaces
> > > instead of just one?
> > 
> > I would like to make udev creating MTD devices, instead of creating them
> > by hands. Adding MTD to LDM would anyway introduce corresponding sysfs
> > files, right? This means we would have one more interface anyway.
> 
> Could be useful, I don't mind you sending a patch.  However, does this
> means that MEMGETINFO64 or some other ioctl should not be done?  Should
> flash_erase open, read and close 8 seperate files instead of doing a
> single ioctl?

It's hardly a fast path. And we don't have to worry about the fact that
it's non-atomic -- these things aren't exactly _changing_ over time.

> And should our support for large devices wait for the sysfs support
> that has been talked about and not done for about two years already?

You whine too much, Jörn. It doesn't take very long, as a proof of
concept, to add some attributes to the existing class support in
mtdchar.c ...

--- drivers/mtd/mtdchar.c~	2008-07-13 22:51:29.000000000 +0100
+++ drivers/mtd/mtdchar.c	2008-08-29 13:15:08.000000000 +0100
@@ -22,12 +22,32 @@
 
 static struct class *mtd_class;
 
+static ssize_t mtd_show_size(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct mtd_info *mtd = dev_get_drvdata(dev);
+	return sprintf(buf, "0x%x\n", mtd->size);
+}
+static ssize_t mtd_show_erasesize(struct device *dev, struct device_attribute *attr,
+				  char *buf)
+{
+	struct mtd_info *mtd = dev_get_drvdata(dev);
+	return sprintf(buf, "0x%x\n", mtd->erasesize);
+}
+
+static DEVICE_ATTR(size, S_IRUGO, mtd_show_size, NULL);
+static DEVICE_ATTR(erasesize, S_IRUGO, mtd_show_erasesize, NULL);
+
 static void mtd_notify_add(struct mtd_info* mtd)
 {
+	struct device *dev;
 	if (!mtd)
 		return;
 
-	device_create(mtd_class, NULL, MKDEV(MTD_CHAR_MAJOR, mtd->index*2), "mtd%d", mtd->index);
+	dev = device_create(mtd_class, NULL, MKDEV(MTD_CHAR_MAJOR, mtd->index*2), "mtd%d", mtd->index);
+	dev_set_drvdata(dev, mtd);
+	device_create_file(dev, &dev_attr_size);
+	device_create_file(dev, &dev_attr_erasesize);
 
 	device_create(mtd_class, NULL,
 		      MKDEV(MTD_CHAR_MAJOR, mtd->index*2+1), "mtd%dro", mtd->index);


Ok, so it shouldn't be only for mtdchar -- it should be generic, so we
should shift some of that into the mtd core code. And we should let
people hook up the 'parent' correctly, and there are a few other things
we should do to tidy it up. But it isn't exactly hard.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[Trent Piepho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 1012 bytes --]

On Wed, 27 Aug 2008, Artem Bityutskiy wrote:
> > On Wed, 2008-08-27 at 16:34 +0200, Jörn Engel wrote:
> > They certainly can, but should they?  There may be some reason to prefer
> > sysfs that should be self-evident - except that I'm a bit thick and seem
> > to need it spelled out.  Or maybe I've just become disillusioned with
> > the practice of replacing crappy interfaces (ioctl here) with other
> > crappy interfaces (sysfs here) and having to support both for all
> > eternity.  sysctl, ioctl, proc, sysfs, debugfs, netlink, ... -
> > individually they all suck in their own peculiar way.  But together they
> > create a mess I no longer dare to name.
> 
> The plus of sysfs I see is that I can add more files to expose more
> information in sysfs, while I can not change MEMGETINFO ioctl. E.g., I
> need to expose sub-page size to user-space, and I cannot do this with
> MEMGETINFO.

You can do this with ioctls, if you designed them with extra space and
versioning from the beginning.