sysfs abuse in recent i2o changes

sysfs abuse in recent i2o changes

[Christoph Hellwig]

drivers/message/i2o/config-osm.c has a function sysfs_create_fops_file,
which creates a sysfs file with supplied file_operations.  This is
pretty much against the sysfs design which only wants simple attributes,
ascsii or for corner cases binary.

Also, if we're going to allow this code it should move to sysfs.  And
stop using lookup_hash directly (use lookup_one_len instead), it'll go
away soon.

Re: sysfs abuse in recent i2o changes

[Markus Lidel]

Hello,

Christoph Hellwig wrote:
> drivers/message/i2o/config-osm.c has a function sysfs_create_fops_file,
> which creates a sysfs file with supplied file_operations.  This is
> pretty much against the sysfs design which only wants simple attributes,
> ascsii or for corner cases binary.

I know, but i hopefully also have a good reason to do so... First, the 
attributes provided through these functions are for accessing the 
firmware... The controller has a little limitation, it could only handle 
64 blocks, but sysfs only have 4k...

Now there are two options:

1) when writing: read a 64k block, merge it with the 4k block and write 
it back, when reading: read a 64k block and only return the needed 4k block.

2) extend the sysfs attribute to allow 64k blocks

IMHO the first is not a very good solution, because for a 64k block it 
has to be written 16 times...

Of course if someone finds a better solution i would be glad to hear 
about it...

Thank you very much.


Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: Markus.Lidel@shadowconnect.com
URL:    http://www.shadowconnect.com

Re: sysfs abuse in recent i2o changes

[Greg KH]

On Tue, Jun 28, 2005 at 05:02:41PM +0200, Markus Lidel wrote:
> I know, but i hopefully also have a good reason to do so... First, the 
> attributes provided through these functions are for accessing the 
> firmware... The controller has a little limitation, it could only handle 
> 64 blocks, but sysfs only have 4k...
> 
> Now there are two options:
> 
> 1) when writing: read a 64k block, merge it with the 4k block and write 
> it back, when reading: read a 64k block and only return the needed 4k block.
> 
> 2) extend the sysfs attribute to allow 64k blocks
> 
> IMHO the first is not a very good solution, because for a 64k block it 
> has to be written 16 times...
> 
> Of course if someone finds a better solution i would be glad to hear 
> about it...

Use the binary file interface of sysfs, which was written exactly for
this kind of thing. :)

thanks,

greg k-h

Re: sysfs abuse in recent i2o changes

[Greg KH]

On Tue, Jun 28, 2005 at 08:08:20PM +0200, Markus Lidel wrote:
> Hello,
> 
> Greg KH wrote:
> >On Tue, Jun 28, 2005 at 05:02:41PM +0200, Markus Lidel wrote:
> >>I know, but i hopefully also have a good reason to do so... First, the 
> >>attributes provided through these functions are for accessing the 
> >>firmware... The controller has a little limitation, it could only handle 
> >>64 blocks, but sysfs only have 4k...
> >>Now there are two options:
> >>1) when writing: read a 64k block, merge it with the 4k block and write 
> >>it back, when reading: read a 64k block and only return the needed 4k 
> >>block.
> >>2) extend the sysfs attribute to allow 64k blocks
> >>IMHO the first is not a very good solution, because for a 64k block it 
> >>has to be written 16 times...
> >>Of course if someone finds a better solution i would be glad to hear 
> >>about it...
> >Use the binary file interface of sysfs, which was written exactly for
> >this kind of thing. :)
> 
> Oh i tried to use the binary interface, but i haven't found a way to 
> increase the block size beyond 4k, could you please tell me how i could 
> adjust it, or where i could read about it?
> 
> Thank you very much!

Your code should not care about the block size of the data given to you,
as userspace could be giving you 1 byte at a time.  Buffer it up
yourself and then write it out to the device when needed.

But if you are doing this for firmware, then please use the kernel
firmware interface, it does all of the buffering for you.

Either way, having your own file_ops in sysfs is not allowed.

thanks,

greg k-h

Re: sysfs abuse in recent i2o changes

[Markus Lidel]

Hello,

Greg KH wrote:
> On Tue, Jun 28, 2005 at 05:02:41PM +0200, Markus Lidel wrote:
>>I know, but i hopefully also have a good reason to do so... First, the 
>>attributes provided through these functions are for accessing the 
>>firmware... The controller has a little limitation, it could only handle 
>>64 blocks, but sysfs only have 4k...
>>Now there are two options:
>>1) when writing: read a 64k block, merge it with the 4k block and write 
>>it back, when reading: read a 64k block and only return the needed 4k block.
>>2) extend the sysfs attribute to allow 64k blocks
>>IMHO the first is not a very good solution, because for a 64k block it 
>>has to be written 16 times...
>>Of course if someone finds a better solution i would be glad to hear 
>>about it...
> Use the binary file interface of sysfs, which was written exactly for
> this kind of thing. :)

Oh i tried to use the binary interface, but i haven't found a way to 
increase the block size beyond 4k, could you please tell me how i could 
adjust it, or where i could read about it?

Thank you very much!


Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: Markus.Lidel@shadowconnect.com
URL:    http://www.shadowconnect.com

Re: sysfs abuse in recent i2o changes

[Markus Lidel]

Hello,

Greg KH wrote:
> On Tue, Jun 28, 2005 at 08:08:20PM +0200, Markus Lidel wrote:
>>Greg KH wrote:
>>>On Tue, Jun 28, 2005 at 05:02:41PM +0200, Markus Lidel wrote:
>>>>I know, but i hopefully also have a good reason to do so... First, the 
>>>>attributes provided through these functions are for accessing the 
>>>>firmware... The controller has a little limitation, it could only handle 
>>>>64 blocks, but sysfs only have 4k...
>>>>Now there are two options:
>>>>1) when writing: read a 64k block, merge it with the 4k block and write 
>>>>it back, when reading: read a 64k block and only return the needed 4k 
>>>>block.
>>>>2) extend the sysfs attribute to allow 64k blocks
>>>>IMHO the first is not a very good solution, because for a 64k block it 
>>>>has to be written 16 times...
>>>>Of course if someone finds a better solution i would be glad to hear 
>>>>about it...
>>>Use the binary file interface of sysfs, which was written exactly for
>>>this kind of thing. :)
>>Oh i tried to use the binary interface, but i haven't found a way to 
>>increase the block size beyond 4k, could you please tell me how i could 
>>adjust it, or where i could read about it?
> Your code should not care about the block size of the data given to you,
> as userspace could be giving you 1 byte at a time.  Buffer it up

The driver don't care about the block size but the device do... Of course 
you could send 1 byte at a time, but if you do so the controller blows up...

> yourself and then write it out to the device when needed.

Yep, but this way there is much more code needed to handle it, and also 
the memory for the buffer may be used longer then just for the read/write...

> But if you are doing this for firmware, then please use the kernel
> firmware interface, it does all of the buffering for you.

In my case the interface is for updating the firmware and also to access 
the configuration settings of the controller... The way to retrieve/store 
the firmware and the configuration settings is the same, so i still have 
to implement the sysfs attributes for the configuration settings...

> Either way, having your own file_ops in sysfs is not allowed.

That was the reason i not changed sysfs and only used it in my own 
module, because i see that it is only needed because of the limitations 
of the hardware...

Thank you very much.


Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: Markus.Lidel@shadowconnect.com
URL:    http://www.shadowconnect.com

Re: sysfs abuse in recent i2o changes

[Greg KH]

On Wed, Jun 29, 2005 at 10:33:03AM +0200, Markus Lidel wrote:
> Hello,
> 
> Greg KH wrote:
> >On Tue, Jun 28, 2005 at 08:08:20PM +0200, Markus Lidel wrote:
> >>Greg KH wrote:
> >>>On Tue, Jun 28, 2005 at 05:02:41PM +0200, Markus Lidel wrote:
> >>>>I know, but i hopefully also have a good reason to do so... First, the 
> >>>>attributes provided through these functions are for accessing the 
> >>>>firmware... The controller has a little limitation, it could only 
> >>>>handle 64 blocks, but sysfs only have 4k...
> >>>>Now there are two options:
> >>>>1) when writing: read a 64k block, merge it with the 4k block and write 
> >>>>it back, when reading: read a 64k block and only return the needed 4k 
> >>>>block.
> >>>>2) extend the sysfs attribute to allow 64k blocks
> >>>>IMHO the first is not a very good solution, because for a 64k block it 
> >>>>has to be written 16 times...
> >>>>Of course if someone finds a better solution i would be glad to hear 
> >>>>about it...
> >>>Use the binary file interface of sysfs, which was written exactly for
> >>>this kind of thing. :)
> >>Oh i tried to use the binary interface, but i haven't found a way to 
> >>increase the block size beyond 4k, could you please tell me how i could 
> >>adjust it, or where i could read about it?
> >Your code should not care about the block size of the data given to you,
> >as userspace could be giving you 1 byte at a time.  Buffer it up
> 
> The driver don't care about the block size but the device do... Of course 
> you could send 1 byte at a time, but if you do so the controller blows up...

Then you need to prevent this in the kernel.

> >yourself and then write it out to the device when needed.
> 
> Yep, but this way there is much more code needed to handle it, and also 
> the memory for the buffer may be used longer then just for the read/write...

No, use the firmware interface, that's what it is there for.

> >But if you are doing this for firmware, then please use the kernel
> >firmware interface, it does all of the buffering for you.
> 
> In my case the interface is for updating the firmware and also to access 
> the configuration settings of the controller... The way to retrieve/store 
> the firmware and the configuration settings is the same, so i still have 
> to implement the sysfs attributes for the configuration settings...

Ok, then use the binary file, and then buffer it up properly yourself,
like the firmware interface does.

> >Either way, having your own file_ops in sysfs is not allowed.
> 
> That was the reason i not changed sysfs and only used it in my own 
> module, because i see that it is only needed because of the limitations 
> of the hardware...

No, please don't do this, either use the binary file interface, or the
firmware one.  Don't use your own file_ops.

thanks,

greg k-h

Re: sysfs abuse in recent i2o changes

[Markus Lidel]

Hello,

Greg KH wrote:
> On Wed, Jun 29, 2005 at 10:33:03AM +0200, Markus Lidel wrote:
> 
>>Hello,
>>
>>Greg KH wrote:
>>
>>>On Tue, Jun 28, 2005 at 08:08:20PM +0200, Markus Lidel wrote:
>>>
>>>>Greg KH wrote:
>>>>
>>>>>On Tue, Jun 28, 2005 at 05:02:41PM +0200, Markus Lidel wrote:
>>>>>
>>>>>>I know, but i hopefully also have a good reason to do so... First, the 
>>>>>>attributes provided through these functions are for accessing the 
>>>>>>firmware... The controller has a little limitation, it could only 
>>>>>>handle 64 blocks, but sysfs only have 4k...
>>>>>>Now there are two options:
>>>>>>1) when writing: read a 64k block, merge it with the 4k block and write 
>>>>>>it back, when reading: read a 64k block and only return the needed 4k 
>>>>>>block.
>>>>>>2) extend the sysfs attribute to allow 64k blocks
>>>>>>IMHO the first is not a very good solution, because for a 64k block it 
>>>>>>has to be written 16 times...
>>>>>>Of course if someone finds a better solution i would be glad to hear 
>>>>>>about it...
>>>>>
>>>>>Use the binary file interface of sysfs, which was written exactly for
>>>>>this kind of thing. :)
>>>>
>>>>Oh i tried to use the binary interface, but i haven't found a way to 
>>>>increase the block size beyond 4k, could you please tell me how i could 
>>>>adjust it, or where i could read about it?
>>>
>>>Your code should not care about the block size of the data given to you,
>>>as userspace could be giving you 1 byte at a time.  Buffer it up
>>
>>The driver don't care about the block size but the device do... Of course 
>>you could send 1 byte at a time, but if you do so the controller blows up...
> 
> Then you need to prevent this in the kernel.
> 
>>>yourself and then write it out to the device when needed.
>>
>>Yep, but this way there is much more code needed to handle it, and also 
>>the memory for the buffer may be used longer then just for the read/write...
> 
> No, use the firmware interface, that's what it is there for.
> 
> 
>>>But if you are doing this for firmware, then please use the kernel
>>>firmware interface, it does all of the buffering for you.
>>
>>In my case the interface is for updating the firmware and also to access 
>>the configuration settings of the controller... The way to retrieve/store 
>>the firmware and the configuration settings is the same, so i still have 
>>to implement the sysfs attributes for the configuration settings...
> 
> Ok, then use the binary file, and then buffer it up properly yourself,
> like the firmware interface does.
> 
>>>Either way, having your own file_ops in sysfs is not allowed.
>>
>>That was the reason i not changed sysfs and only used it in my own 
>>module, because i see that it is only needed because of the limitations 
>>of the hardware...
> 
> No, please don't do this, either use the binary file interface, or the
> firmware one.  Don't use your own file_ops.

OK, this doesn't work, because of side effects with buffering, but i'll 
remove the new interface, and replace it with the old ioctl interface again.

Thank you very much for your comments.


Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: Markus.Lidel@shadowconnect.com
URL:    http://www.shadowconnect.com

Re: sysfs abuse in recent i2o changes

[Christoph Hellwig]

So folks, this is still in 2.6.13-rc4, shouldn't we pull it out so we
don't add an interface soon to be removed again to 2.6.13?

Re: sysfs abuse in recent i2o changes

[Markus Lidel]

Hello,

Christoph Hellwig wrote:
> So folks, this is still in 2.6.13-rc4, shouldn't we pull it out so we

Yep, sorry... I'll try to send the patch during next week...

> don't add an interface soon to be removed again to 2.6.13?

The interface will still be available, because IMHO it fits better then 
the old ioctl based one... But until the necessary functions for the 
interface are available, i'll provide a separate patch, which could be 
downloaded at the I2O website...

Sorry for my delay :-(



Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: Markus.Lidel@shadowconnect.com
URL:    http://www.shadowconnect.com

Re: sysfs abuse in recent i2o changes

[Greg KH]

On Sun, Jul 31, 2005 at 05:40:29PM +0200, Markus Lidel wrote:
> Hello,
> 
> Christoph Hellwig wrote:
> >So folks, this is still in 2.6.13-rc4, shouldn't we pull it out so we
> 
> Yep, sorry... I'll try to send the patch during next week...
> 
> >don't add an interface soon to be removed again to 2.6.13?
> 
> The interface will still be available, because IMHO it fits better then 
> the old ioctl based one... But until the necessary functions for the 
> interface are available, i'll provide a separate patch, which could be 
> downloaded at the I2O website...

What "necessary functions for the interface"?  Remember, you are not
using sysfs properly, so please fix your code to not do that anymore.

thanks,

greg k-h