Re: Seeking advice on "monkey patching" a driver
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! ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: Seeking advice on "monkey patching" a driver
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 ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: Seeking advice on "monkey patching" a driver
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! ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: Seeking advice on "monkey patching" a driver
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 ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: Seeking advice on "monkey patching" a driver
On Thu, Jul 1, 2021 at 2:03 PM 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 > * (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. ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: Seeking advice on "monkey patching" a driver
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! ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: Seeking advice on "monkey patching" a driver
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 ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Seeking advice on "monkey patching" a driver
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! ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
