On Monday, July 30, 2012, Borislav Petkov wrote:
> On Sun, Jul 29, 2012 at 09:31:53PM +0200, Witold Szczeponik wrote:
> > the aim is to select a PNP ACPI option where resources can be disabled
> > (or are not needed). E.g., the parallel port of the 600E can be used
> > with and without IRQ lines. The means to allow for this is to use the
> > sysfs interface to select disabled resources (just like any other
> > resource value). In https://lkml.org/lkml/2011/7/3/41, I used the
> > following example:
> >
> > echo disable > /sys/bus/pnp/devices/$device/resources
> > echo clear > /sys/bus/pnp/devices/$device/resources
> > echo set irq disabled > /sys/bus/pnp/devices/$device/resources
> > echo fill > /sys/bus/pnp/devices/$device/resources
> > echo activate > /sys/bus/pnp/devices/$device/resources
> >
> > The third line is made possible by the patch series. All other
> > lines are already implemented.
>
> Shouldn't this be rather "disable_irq" or something which is a single
> word and thus would simplify parsing a lot?
Or just "irq", which isn't going to be confused with anything else it seems.
Thanks,
Rafael
On 02/08/12 22:09, Rafael J. Wysocki wrote:
> On Monday, July 30, 2012, Borislav Petkov wrote:
>> On Sun, Jul 29, 2012 at 09:31:53PM +0200, Witold Szczeponik wrote:
>>> the aim is to select a PNP ACPI option where resources can be disabled
>>> (or are not needed). E.g., the parallel port of the 600E can be used
>>> with and without IRQ lines. The means to allow for this is to use the
>>> sysfs interface to select disabled resources (just like any other
>>> resource value). In https://lkml.org/lkml/2011/7/3/41, I used the
>>> following example:
>>>
>>> echo disable > /sys/bus/pnp/devices/$device/resources
>>> echo clear > /sys/bus/pnp/devices/$device/resources
>>> echo set irq disabled > /sys/bus/pnp/devices/$device/resources
>>> echo fill > /sys/bus/pnp/devices/$device/resources
>>> echo activate > /sys/bus/pnp/devices/$device/resources
>>>
>>> The third line is made possible by the patch series. All other
>>> lines are already implemented.
>>
>> Shouldn't this be rather "disable_irq" or something which is a single
>> word and thus would simplify parsing a lot?
>
> Or just "irq", which isn't going to be confused with anything else it seems.
>
> Thanks,
> Rafael
>
Hi Rafael,
the code in "drivers/pnp/interface.c" implements a (non-trivial) interface
which accepts the keywords "disable", "activate", "fill", "auto", "clear",
and "get" as simple, one word commands. The remaining "set" command is
more complex, for it determines which resource is to be set ("io", "mem",
"irq", "dma", and "bus"), followed by the actual value(s) of this resource
(e.g., "0x0200-0x021f", or "7").
The patch series allows to use the term "disabled" or "<none>" as a
resource value (c.f. my example above) when needed (c.f. my motivation for
the patch series).
We could, of course, change the parser in "interface.c", but this would
change the ABI, I am afraid. Something that I'd rather not do...
I hope, this makes the scope of the patch series clear(er).
--- Witold
On Thursday, August 02, 2012, Witold Szczeponik wrote:
> On 02/08/12 22:09, Rafael J. Wysocki wrote:
> > On Monday, July 30, 2012, Borislav Petkov wrote:
> >> On Sun, Jul 29, 2012 at 09:31:53PM +0200, Witold Szczeponik wrote:
> >>> the aim is to select a PNP ACPI option where resources can be disabled
> >>> (or are not needed). E.g., the parallel port of the 600E can be used
> >>> with and without IRQ lines. The means to allow for this is to use the
> >>> sysfs interface to select disabled resources (just like any other
> >>> resource value). In https://lkml.org/lkml/2011/7/3/41, I used the
> >>> following example:
> >>>
> >>> echo disable > /sys/bus/pnp/devices/$device/resources
> >>> echo clear > /sys/bus/pnp/devices/$device/resources
> >>> echo set irq disabled > /sys/bus/pnp/devices/$device/resources
> >>> echo fill > /sys/bus/pnp/devices/$device/resources
> >>> echo activate > /sys/bus/pnp/devices/$device/resources
> >>>
> >>> The third line is made possible by the patch series. All other
> >>> lines are already implemented.
> >>
> >> Shouldn't this be rather "disable_irq" or something which is a single
> >> word and thus would simplify parsing a lot?
> >
> > Or just "irq", which isn't going to be confused with anything else it seems.
> >
> > Thanks,
> > Rafael
> >
>
> Hi Rafael,
>
> the code in "drivers/pnp/interface.c" implements a (non-trivial) interface
> which accepts the keywords "disable", "activate", "fill", "auto", "clear",
> and "get" as simple, one word commands. The remaining "set" command is
> more complex, for it determines which resource is to be set ("io", "mem",
> "irq", "dma", and "bus"), followed by the actual value(s) of this resource
> (e.g., "0x0200-0x021f", or "7").
>
> The patch series allows to use the term "disabled" or "<none>" as a
> resource value (c.f. my example above) when needed (c.f. my motivation for
> the patch series).
>
> We could, of course, change the parser in "interface.c", but this would
> change the ABI, I am afraid. Something that I'd rather not do...
Still, you _are_ doing that by extending the ABI, aren't you?
> I hope, this makes the scope of the patch series clear(er).
Yes, it does, thanks.
My opinion is that the whole interface is wrong and should be changed. How to
do that is a different matter that would require some consideration. Perhaps
the least painful way would be to add a new, hopefully better, interface along
with the old one and then deprecate the latter at one point.
Now, since I don't like the existing interface, I'd prefer it not to be
extended.
Thanks,
Rafael
On 02/08/12 23:40, Rafael J. Wysocki wrote:
> On Thursday, August 02, 2012, Witold Szczeponik wrote:
>> On 02/08/12 22:09, Rafael J. Wysocki wrote:
>>> On Monday, July 30, 2012, Borislav Petkov wrote:
>>>> On Sun, Jul 29, 2012 at 09:31:53PM +0200, Witold Szczeponik wrote:
[... snip ...]
>>>>
>>>> Shouldn't this be rather "disable_irq" or something which is a single
>>>> word and thus would simplify parsing a lot?
>>>
>>> Or just "irq", which isn't going to be confused with anything else it seems.
>>>
>>> Thanks,
>>> Rafael
>>>
>>
>> Hi Rafael,
>>
>> the code in "drivers/pnp/interface.c" implements a (non-trivial) interface
>> which accepts the keywords "disable", "activate", "fill", "auto", "clear",
>> and "get" as simple, one word commands. The remaining "set" command is
>> more complex, for it determines which resource is to be set ("io", "mem",
>> "irq", "dma", and "bus"), followed by the actual value(s) of this resource
>> (e.g., "0x0200-0x021f", or "7").
>>
>> The patch series allows to use the term "disabled" or "<none>" as a
>> resource value (c.f. my example above) when needed (c.f. my motivation for
>> the patch series).
>>
>> We could, of course, change the parser in "interface.c", but this would
>> change the ABI, I am afraid. Something that I'd rather not do...
>
> Still, you _are_ doing that by extending the ABI, aren't you?
As the special value "disabled" is available as of these patches, one could
consider this an extension. I agree.
>
>> I hope, this makes the scope of the patch series clear(er).
>
> Yes, it does, thanks.
>
> My opinion is that the whole interface is wrong and should be changed. How to
> do that is a different matter that would require some consideration. Perhaps
> the least painful way would be to add a new, hopefully better, interface along
> with the old one and then deprecate the latter at one point.
Personally, I too think that the PNP ABI in sysfs has its rough edges. However,
as with the deprecation of any existing ABI, this would require a new ABI first,
then some time where the old and new ABI live in co-existence, and then to remove
the currently available ABI.
>
> Now, since I don't like the existing interface, I'd prefer it not to be
> extended.
The current ABI does not allow for the kernel to run on my hardware: this is
a/the problem. The proposed extension fixes the problem.
While I agree with your first statement, for the time being I do not see a
better solution other than to extend the ABI.
At this point I am repeating my "call for comments" to the community. :-)
--- Witold
>
> Thanks,
> Rafael
>