Seeking advice on "monkey patching" a driver

Seeking advice on "monkey patching" a driver

[Ian Pilcher]

I maintain a couple of out-of-tree modules that enable "mainstream"
distributions to be used on the Thecus N5550 NAS.

  https://github.com/ipilcher/n5550/tree/master/modules

The disk activity LEDs in this NAS are software controlled, so the
n5550_ahci_leds module exists to "inject" a wrapper around libahci's
qc_issue() function which triggers the correct LED (if any).

 
https://github.com/ipilcher/n5550/blob/25538096fffd7942be8b7f2c66af580620a422b6/modules/n5550_ahci_leds.c#L225

It's certainly a hack, but it makes use of the modules much simpler, as
there's no need to patch and rebuild libahci, override the distro-
provided module, etc.  As long as modprobe is configured to load
n5550_ahci_leds immediately after libahci and before any consumers of
libahci are loaded, things "just work".

The current version of n5550_ahci_leds attempts to ensure that no
libahci consumers are loaded before it modifies libahci's
ahci_ops.qc_issue.  It does this by:

   * locking module_mutex
   * getting a reference to the libahci module (with find_module())
   * checking libahci's reference count
   * grabbing a reference to *itself* to prevent itself from being
     unloaded
   * modifying ahci_ops.qc_issue
   * unlocking module_mutex

(There similar logic in the n5550_ahci_leds_enabled_store function to
reverse the modifications, if no other libahci consumers are loaded.)

It's very possible that some or all of these precautions are
unnecessary (or that they're inadequate).  I am most definitely not an
expert at kernel development or the details of the kernel's module
loading mechanism.  I and few others have, however, been successfully
using these modules for a number of years.

I've just discovered that neither module_mutex nor find_module() are
available in recent kernels, and I'm unsure how to proceed.  The two
options that I've been able to think of thus far are:

* YOLO!  I can simply remove the checks from the module and rely on user
   space to ensure that n5550_ahci_leds is loaded before any libahci
   consumers load (i.e. before udevd starts).

* kprobes - I have a feeling that this is the "correct" way to do this
   (and it would have the benefit of working even if libahci or its
   consumers aren't built as modules).  OTOH, it isn't clear how I would
   go about accessing the arguments passed into the function without
   JProbes, and I'm not thrilled with the idea of adding additional
   overhead and/or locking to the disk I/O path.

I'd really appreciate any thoughts, advice, ideas, links etc.

Thanks!

-- 
========================================================================
                  In Soviet Russia, Google searches you!
========================================================================

Re: Seeking advice on "monkey patching" a driver

[Greg KH]

On Thu, Jul 01, 2021 at 11:25:49AM -0500, Ian Pilcher wrote:
> I maintain a couple of out-of-tree modules that enable "mainstream"
> distributions to be used on the Thecus N5550 NAS.
> 
>  https://github.com/ipilcher/n5550/tree/master/modules
> 
> The disk activity LEDs in this NAS are software controlled, so the
> n5550_ahci_leds module exists to "inject" a wrapper around libahci's
> qc_issue() function which triggers the correct LED (if any).
> 
> 
> https://github.com/ipilcher/n5550/blob/25538096fffd7942be8b7f2c66af580620a422b6/modules/n5550_ahci_leds.c#L225
> 
> It's certainly a hack, but it makes use of the modules much simpler, as
> there's no need to patch and rebuild libahci, override the distro-
> provided module, etc.  As long as modprobe is configured to load
> n5550_ahci_leds immediately after libahci and before any consumers of
> libahci are loaded, things "just work".
> 
> The current version of n5550_ahci_leds attempts to ensure that no
> libahci consumers are loaded before it modifies libahci's
> ahci_ops.qc_issue.  It does this by:
> 
>   * locking module_mutex
>   * getting a reference to the libahci module (with find_module())
>   * checking libahci's reference count
>   * grabbing a reference to *itself* to prevent itself from being
>     unloaded
>   * modifying ahci_ops.qc_issue
>   * unlocking module_mutex

Oh that's horrible, please no, do not do that :)

> (There similar logic in the n5550_ahci_leds_enabled_store function to
> reverse the modifications, if no other libahci consumers are loaded.)
> 
> It's very possible that some or all of these precautions are
> unnecessary (or that they're inadequate).  I am most definitely not an
> expert at kernel development or the details of the kernel's module
> loading mechanism.  I and few others have, however, been successfully
> using these modules for a number of years.
> 
> I've just discovered that neither module_mutex nor find_module() are
> available in recent kernels, and I'm unsure how to proceed.  The two
> options that I've been able to think of thus far are:
> 
> * YOLO!  I can simply remove the checks from the module and rely on user
>   space to ensure that n5550_ahci_leds is loaded before any libahci
>   consumers load (i.e. before udevd starts).
> 
> * kprobes - I have a feeling that this is the "correct" way to do this
>   (and it would have the benefit of working even if libahci or its
>   consumers aren't built as modules).  OTOH, it isn't clear how I would
>   go about accessing the arguments passed into the function without
>   JProbes, and I'm not thrilled with the idea of adding additional
>   overhead and/or locking to the disk I/O path.

How about a third option, the correct one:
	- submit your code changes upstream and they get merged into the
	  main kernel tree and no monkeypatching is ever needed at all!

Have you submitted your changes upstream to the existing drivers?  What
is preventing that from happening today?

thanks,

greg k-h

Re: Seeking advice on "monkey patching" a driver

[Ian Pilcher]

On 7/1/21 12:59 PM, Greg KH wrote:
> Oh that's horrible, please no, do not do that :)

Indeed it is, but it works, and it meets my main objective, which is to
allow the use of distribution kernel packages and modules.

> How about a third option, the correct one:
> 	- submit your code changes upstream and they get merged into the
> 	  main kernel tree and no monkeypatching is ever needed at all!
> 
> Have you submitted your changes upstream to the existing drivers?  What
> is preventing that from happening today?

There are a couple of reasons that I've never attempted to do this.

* Scope of work - Currently, there is simply no mechanism to call an LED
   trigger from the ahci or libahci modules, presumably because this is
   something that  really ought to be done by the hardware.  So I would
   have to add some sort of generic framework to associate LED triggers
   with AHCI ports.

   I probably also don't really have the knowledge to do this.  I am not
   familiar with locking, memory management, etc. in the kernel.  Just
   because my "hack" works on a specific 2-core NAS doesn't mean that it
   won't cause all sorts of breakage on a higher-performance system with
   more parallelism.

* (Probable) lack of upstream interest - As I mentioned, disk activity
   LEDs really ought to be handled by the hardware.  I don't know of any
   other system that suffers from this particular limitation.  So this
   is a very, very niche use case.  (Most users of this hardware use the
   manufacturer's "firmware".)

   I did ask about this on the linux-ide mailing list long ago when I
   first wrote the modules, but I don't think that I ever received a
   response, which reinforces my belief that upstream isn't likely to be
   receptive.

I've invested significant time in kernel patches in the past, only to
see them ultimately not be accepted, so I would need to know that
upstream was truly interested in such a feature before I would consider
making such a commitment.

-- 
========================================================================
                  In Soviet Russia, Google searches you!
========================================================================

Re: Seeking advice on "monkey patching" a driver

[jim.cromie]

On Thu, Jul 1, 2021 at 2:03 PM Ian Pilcher <arequipeno@gmail.com> wrote:
>
> On 7/1/21 12:59 PM, Greg KH wrote:
> > Oh that's horrible, please no, do not do that :)
>
> Indeed it is, but it works, and it meets my main objective, which is to
> allow the use of distribution kernel packages and modules.
>
> > How about a third option, the correct one:
> >       - submit your code changes upstream and they get merged into the
> >         main kernel tree and no monkeypatching is ever needed at all!
> >
> > Have you submitted your changes upstream to the existing drivers?  What
> > is preventing that from happening today?
>
> There are a couple of reasons that I've never attempted to do this.
>
> * Scope of work - Currently, there is simply no mechanism to call an LED
> * (Probable) lack of upstream interest - As I mentioned, disk activity
>    LEDs really ought to be handled by the hardware.

Are LEDs really that important?
Unless theyre rigged intrinsically into the operation, it seems tertiary



  I don't know of any
>    other system that suffers from this particular limitation.  So this
>    is a very, very niche use case.  (Most users of this hardware use the
>    manufacturer's "firmware".)
>
>    I did ask about this on the linux-ide mailing list long ago when I
>    first wrote the modules, but I don't think that I ever received a
>    response, which reinforces my belief that upstream isn't likely to be
>    receptive.
>

theres a firehose of patches.

FWIW, now robots watch the list, and will grind your patches on lots
of configs. arches



> I've invested significant time in kernel patches in the past, only to
> see them ultimately not be accepted, so I would need to know that
> upstream was truly interested in such a feature before I would consider
> making such a commitment.
>

no guarantees, but there is staging. (here, more or less)
provisional home for code while quality develops
once youre in-tree, warts and all (to some extent, I dont know)
you may well get help (patches) improving it, surely lots of feedback.



elsewhere, nobody knows it exists.

Re: Seeking advice on "monkey patching" a driver

[Greg KH]

On Thu, Jul 01, 2021 at 03:03:12PM -0500, Ian Pilcher wrote:
> On 7/1/21 12:59 PM, Greg KH wrote:
> > Oh that's horrible, please no, do not do that :)
> 
> Indeed it is, but it works, and it meets my main objective, which is to
> allow the use of distribution kernel packages and modules.
> 
> > How about a third option, the correct one:
> > 	- submit your code changes upstream and they get merged into the
> > 	  main kernel tree and no monkeypatching is ever needed at all!
> > 
> > Have you submitted your changes upstream to the existing drivers?  What
> > is preventing that from happening today?
> 
> There are a couple of reasons that I've never attempted to do this.
> 
> * Scope of work - Currently, there is simply no mechanism to call an LED
>   trigger from the ahci or libahci modules, presumably because this is
>   something that  really ought to be done by the hardware.  So I would
>   have to add some sort of generic framework to associate LED triggers
>   with AHCI ports.
> 
>   I probably also don't really have the knowledge to do this.  I am not
>   familiar with locking, memory management, etc. in the kernel.  Just
>   because my "hack" works on a specific 2-core NAS doesn't mean that it
>   won't cause all sorts of breakage on a higher-performance system with
>   more parallelism.

Why are ahci devices somehow "special" here?  Just add a trigger to the
ahci core for LEDs and all should "just work".  We've done that for many
subsystems already.

> * (Probable) lack of upstream interest - As I mentioned, disk activity
>   LEDs really ought to be handled by the hardware.  I don't know of any
>   other system that suffers from this particular limitation.  So this
>   is a very, very niche use case.  (Most users of this hardware use the
>   manufacturer's "firmware".)

Are you sure we don't already have LED triggers for disk activity?  Have
you tried the ledtrig-disk.c driver?  It says it works on ATA devices,
no reason it can't also work for other device types.

>   I did ask about this on the linux-ide mailing list long ago when I
>   first wrote the modules, but I don't think that I ever received a
>   response, which reinforces my belief that upstream isn't likely to be
>   receptive.
> 
> I've invested significant time in kernel patches in the past, only to
> see them ultimately not be accepted, so I would need to know that
> upstream was truly interested in such a feature before I would consider
> making such a commitment.

That's not fair, there is no way anyone can promise anyone that their
patches will be accepted, _before_ anyone sees them.  What would _you_
do if you were in the kernel maintainer's position and read something
like this?

good luck!

greg k-h

Re: Seeking advice on "monkey patching" a driver

[Ian Pilcher]

On 7/1/21 11:31 PM, Greg KH wrote:
> Why are ahci devices somehow "special" here?  Just add a trigger to the
> ahci core for LEDs and all should "just work".  We've done that for many
> subsystems already.

It's more complicated than that, as it would need to be a separate
trigger for each drive (ATA port).

> Are you sure we don't already have LED triggers for disk activity?  Have
> you tried the ledtrig-disk.c driver?  It says it works on ATA devices,
> no reason it can't also work for other device types.

I stumbled on that file myself last night.  I either wasn't aware of it
before, or I had forgotten its existence, possibly because it's disabled
(CONFIG_LEDS_TRIGGER_DISK=n).  As mentioned above, I would be looking to
enable "per port" LEDs, so it doesn't work in its current form.  It does
at least give me hope that enhancements might be accepted upstream.

>> I've invested significant time in kernel patches in the past, only to
>> see them ultimately not be accepted, so I would need to know that
>> upstream was truly interested in such a feature before I would consider
>> making such a commitment.
> 
> That's not fair, there is no way anyone can promise anyone that their
> patches will be accepted, _before_ anyone sees them.  What would _you_
> do if you were in the kernel maintainer's position and read something
> like this?

You're right, but that isn't what I intended to say.  Basically, I can't
afford to invest the time in implementing something if the subsystem
maintainers have no interest in the *functionality*, regardless of the
state of the code.  I.e., if the ATA/LED subsystem maintainers think
that  software-controlled disk activity LEDs are stupid and have no
place in the kernel, then code quality is irrelevant, and anything I do
will be a waste of time.

> good luck!

Thanks!

-- 
========================================================================
                  In Soviet Russia, Google searches you!
========================================================================

Re: Seeking advice on "monkey patching" a driver

[Greg KH]

On Fri, Jul 02, 2021 at 07:05:26AM -0500, Ian Pilcher wrote:
> > > I've invested significant time in kernel patches in the past, only to
> > > see them ultimately not be accepted, so I would need to know that
> > > upstream was truly interested in such a feature before I would consider
> > > making such a commitment.
> > 
> > That's not fair, there is no way anyone can promise anyone that their
> > patches will be accepted, _before_ anyone sees them.  What would _you_
> > do if you were in the kernel maintainer's position and read something
> > like this?
> 
> You're right, but that isn't what I intended to say.  Basically, I can't
> afford to invest the time in implementing something if the subsystem
> maintainers have no interest in the *functionality*, regardless of the
> state of the code.  I.e., if the ATA/LED subsystem maintainers think
> that  software-controlled disk activity LEDs are stupid and have no
> place in the kernel, then code quality is irrelevant, and anything I do
> will be a waste of time.

Again, without a real patch, no maintainer or developer will ever say if
they will, or will not, accept such a thing.  That's just not how kernel
development works.  Working patches are what is discussed as that proves
that at least, the idea works.

thanks,

greg k-h

Re: Seeking advice on "monkey patching" a driver

[Ian Pilcher]

On 7/2/21 7:20 AM, Greg KH wrote:
> Again, without a real patch, no maintainer or developer will ever say if
> they will, or will not, accept such a thing.  That's just not how kernel
> development works.  Working patches are what is discussed as that proves
> that at least, the idea works.

And that's certainly their prerogative.  You asked why I hadn't
attempted to submit changes upstream to address my issue, and I provided
my reasons for not doing so.

After several experiences where I've invested a significant (to me)
amount of time, only to ultimately be told that the functionality that I
implemented really wasn't desired, I'm very reluctant to repeat that
experience (and I honestly don't think that's particularly selfish or
unreasonable).

This doesn't mean that I absolutely won't make an attempt to do this.
As I mentioned in my previous note, the existence of ledtrig-disk.c
gives me some hope that changes might be accepted.

I'll just have to see ...

-- 
========================================================================
                  In Soviet Russia, Google searches you!
========================================================================