HSI: hsi: Introducing HSI framework
A message that is not in the archive (sent off-list or not yet received): [email protected]
Review
Quoted 7 lines · Linux Kernel Mailing List
(quoted diff) +7
-0+static void hsi_controller_release(struct device *dev __maybe_unused) +{ +} + +static void hsi_port_release(struct device *dev __maybe_unused) +{ +}
As per the documentation in the kernel tree, I get to mock you mercilessly for doing something as foolish as this. You are not smarter than the kernel and don't think that you got rid of the kernel warning properly by doing this. Do you think that I wrote that code for no good reason? The kernel was being nice and telling you what you did wrong, don't try to fake it out, it's smarter than you are here.
I also get to tell you that you need to fix this up right now, BEFORE 3.4 comes out.
And no, just because you created static struct devices, this is not ok, DO NOT create static struct devices, that's foolish, and even worse, not something you should be doing in the first place, create a real structure, and put a struct device within it please.
And people wonder why kernel maintainers are grumpy.
Quoted 8 lines
(quoted diff) +8
-0+/** + * struct hsi_client - HSI client attached to an HSI port + * @device: Driver model representation of the device + * @tx_cfg: HSI TX configuration + * @rx_cfg: HSI RX configuration + * @hsi_start_rx: Called after incoming wake line goes high + * @hsi_stop_rx: Called after incoming wake line goes low + */
Review
Quoted 10 lines
(quoted diff) +10
-0+struct hsi_client { + struct device device; + struct hsi_config tx_cfg; + struct hsi_config rx_cfg; + void (*hsi_start_rx)(struct hsi_client *cl); + void (*hsi_stop_rx)(struct hsi_client *cl); + /* private: */ + unsigned int pclaimed:1; + struct list_head link; +};
Why do you need another list_head in here? What's wrong with using the device iteration functions that are written for you to use? Don't put the structure on yet-another-list, use what is given to you please, as odds are, you will traverse this list incorrectly (trust me...)
ugh,
greg k-h
Hi Greg,
Thanks for the review.
Quoted 16 lines · ext Greg KH
Review
Quoted 7 lines · Linux Kernel Mailing List
(quoted diff) +7
-0+static void hsi_controller_release(struct device *dev __maybe_unused) +{ +} + +static void hsi_port_release(struct device *dev __maybe_unused) +{ +}As per the documentation in the kernel tree, I get to mock you mercilessly for doing something as foolish as this. You are not smarter than the kernel and don't think that you got rid of the kernel warning properly by doing this. Do you think that I wrote that code for no good reason? The kernel was being nice and telling you what you did wrong, don't try to fake it out, it's smarter than you are here.
Quoted 7 lines
I also get to tell you that you need to fix this up right now, BEFORE 3.4 comes out.
And no, just because you created static struct devices, this is not ok, DO NOT create static struct devices, that's foolish, and even worse, not something you should be doing in the first place, create a real structure, and put a struct device within it please.
That was not really my intention here.
But it is my fault for not keeping up with the documentation. For what is worth, back on the days that the framework was initially written, there was no such documentation and I went for a simple API approach (alloc -> register -> unregister -> free). It also made quite trivial the hsi_port allocation/freeing and device registration.
I'll do the changes asap.
Quoted 18 lines
And people wonder why kernel maintainers are grumpy.
Quoted 15 lines
(quoted diff) +15
-0+/** + * struct hsi_client - HSI client attached to an HSI port + * @device: Driver model representation of the device + * @tx_cfg: HSI TX configuration + * @rx_cfg: HSI RX configuration + * @hsi_start_rx: Called after incoming wake line goes high + * @hsi_stop_rx: Called after incoming wake line goes low + */ +struct hsi_client { + struct device device; + struct hsi_config tx_cfg; + struct hsi_config rx_cfg; + void (*hsi_start_rx)(struct hsi_client *cl); + void (*hsi_stop_rx)(struct hsi_client *cl); + /* private: */
Quoted 9 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ unsigned int pclaimed:1; + struct list_head link; +};Why do you need another
list_headin here? What's wrong with using the device iteration functions that are written for you to use? Don't put the structure on yet-another-list, use what is given to you please, as odds are, you will traverse this list incorrectly (trust me...)
Well I do agree about using the device iteration functions, and way back in the old times of this framework that was the case:
https://lkml.org/lkml/2010/5/7/240
The issue here was that I wanted to allow the hsi_controllers drivers to call hsi_event() in their BHs and ISR to minimize the latency in the communication . And that's not possible if I use the device_for_each_child() in hsi_event(), cause the locking does not protect against interrupt handlers.
But I guess I can go back to the old code and push the issue down to the the hsi controller drivers and force them to use threaded interrupts...
Quoted 3 lines
ugh,
greg k-h
Br,
Carlos Chinea
Quoted 29 lines · Carlos Chinea
Hi Greg,
Thanks for the review.
Quoted 16 lines · ext Greg KH
Review
Quoted 7 lines · Linux Kernel Mailing List
(quoted diff) +7
-0+static void hsi_controller_release(struct device *dev __maybe_unused) +{ +} + +static void hsi_port_release(struct device *dev __maybe_unused) +{ +}As per the documentation in the kernel tree, I get to mock you mercilessly for doing something as foolish as this. You are not smarter than the kernel and don't think that you got rid of the kernel warning properly by doing this. Do you think that I wrote that code for no good reason? The kernel was being nice and telling you what you did wrong, don't try to fake it out, it's smarter than you are here.
Quoted 7 lines
I also get to tell you that you need to fix this up right now, BEFORE 3.4 comes out.
And no, just because you created static struct devices, this is not ok, DO NOT create static struct devices, that's foolish, and even worse, not something you should be doing in the first place, create a real structure, and put a struct device within it please.
Quoted 6 lines
That was not really my intention here.
But it is my fault for not keeping up with the documentation. For what is worth, back on the days that the framework was initially written, there was no such documentation and I went for a simple API approach (alloc -> register -> unregister -> free). It also made quite trivial thehsi_portallocation/freeing and device registration.
When was this code initially written that the documentation, and the kernel, did not show these types of warnings? When you originally posted this in 2010? Sorry, but no, those warnings and documentation have been there for many many years. You obviously were avoiding them by providing empty functions, otherwise you would just have not had them at all.
Quoted 30 lines
Quoted 17 lines
And people wonder why kernel maintainers are grumpy.
Quoted 15 lines
(quoted diff) +15
-0+/** + * struct hsi_client - HSI client attached to an HSI port + * @device: Driver model representation of the device + * @tx_cfg: HSI TX configuration + * @rx_cfg: HSI RX configuration + * @hsi_start_rx: Called after incoming wake line goes high + * @hsi_stop_rx: Called after incoming wake line goes low + */ +struct hsi_client { + struct device device; + struct hsi_config tx_cfg; + struct hsi_config rx_cfg; + void (*hsi_start_rx)(struct hsi_client *cl); + void (*hsi_stop_rx)(struct hsi_client *cl); + /* private: */Quoted 9 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ unsigned int pclaimed:1; + struct list_head link; +};Why do you need another
list_headin here? What's wrong with using the device iteration functions that are written for you to use? Don't put the structure on yet-another-list, use what is given to you please, as odds are, you will traverse this list incorrectly (trust me...)Well I do agree about using the device iteration functions, and way back in the old times of this framework that was the case:
Quoted 7 lines
https://lkml.org/lkml/2010/5/7/240
The issue here was that I wanted to allow the
hsi_controllersdrivers to callhsi_event()in their BHs and ISR to minimize the latency in the communication . And that's not possible if I use thedevice_for_each_child()inhsi_event(), cause the locking does not protect against interrupt handlers.
I don't understand, why would a "client" need to iterate to find other clients in interrupt context? What happens if you loose a device from the system while this is happening? That's why we have a special driver-core-only list functionality to keep all of the locking and iteration sane.
Quoted 2 lines
But I guess I can go back to the old code and push the issue down to the the hsi controller drivers and force them to use threaded interrupts...
I don't understand what you are trying to solve here. Any pointers to where I should look?
thanks,
greg k-h
Quoted 37 lines · ext ext Greg KH
Quoted 29 lines · Carlos Chinea
Hi Greg,
Thanks for the review.
Quoted 16 lines · ext Greg KH
Review
Quoted 7 lines · Linux Kernel Mailing List
(quoted diff) +7
-0+static void hsi_controller_release(struct device *dev __maybe_unused) +{ +} + +static void hsi_port_release(struct device *dev __maybe_unused) +{ +}As per the documentation in the kernel tree, I get to mock you mercilessly for doing something as foolish as this. You are not smarter than the kernel and don't think that you got rid of the kernel warning properly by doing this. Do you think that I wrote that code for no good reason? The kernel was being nice and telling you what you did wrong, don't try to fake it out, it's smarter than you are here.
Quoted 7 lines
I also get to tell you that you need to fix this up right now, BEFORE 3.4 comes out.
And no, just because you created static struct devices, this is not ok, DO NOT create static struct devices, that's foolish, and even worse, not something you should be doing in the first place, create a real structure, and put a struct device within it please.
Quoted 6 lines
That was not really my intention here.
But it is my fault for not keeping up with the documentation. For what is worth, back on the days that the framework was initially written, there was no such documentation and I went for a simple API approach (alloc -> register -> unregister -> free). It also made quite trivial thehsi_portallocation/freeing and device registration.
Quoted 6 lines
When was this code initially written that the documentation, and the kernel, did not show these types of warnings? When you originally posted this in 2010? Sorry, but no, those warnings and documentation have been there for many many years. You obviously were avoiding them by providing empty functions, otherwise you would just have not had them at all.
Yes you are fully right. The documentation was there back in 2.6.32. Totally my bad and I am really really sorry for this stupid comment. And yes the warnings were there.
Quoted 32 lines
Quoted 32 lines
Quoted 17 lines
And people wonder why kernel maintainers are grumpy.
Quoted 15 lines
(quoted diff) +15
-0+/** + * struct hsi_client - HSI client attached to an HSI port + * @device: Driver model representation of the device + * @tx_cfg: HSI TX configuration + * @rx_cfg: HSI RX configuration + * @hsi_start_rx: Called after incoming wake line goes high + * @hsi_stop_rx: Called after incoming wake line goes low + */ +struct hsi_client { + struct device device; + struct hsi_config tx_cfg; + struct hsi_config rx_cfg; + void (*hsi_start_rx)(struct hsi_client *cl); + void (*hsi_stop_rx)(struct hsi_client *cl); + /* private: */Quoted 9 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ unsigned int pclaimed:1; + struct list_head link; +};Why do you need another
list_headin here? What's wrong with using the device iteration functions that are written for you to use? Don't put the structure on yet-another-list, use what is given to you please, as odds are, you will traverse this list incorrectly (trust me...)Well I do agree about using the device iteration functions, and way back in the old times of this framework that was the case:
Quoted 8 lines
Quoted 5 lines
The issue here was that I wanted to allow the
hsi_controllersdrivers to callhsi_event()in their BHs and ISR to minimize the latency in the communication . And that's not possible if I use thedevice_for_each_child()inhsi_event(), cause the locking does not protect against interrupt handlers.I don't understand, why would a "client" need to iterate to find other clients in interrupt context?
Not over other clients. It is the hsi_port which iterates around its attached clients and to signal them the rise and fall of its wake line.
Quoted 5 lines
What happens if you loose a device from
the system while this is happening? That's why we have a special driver-core-only list functionality to keep all of the locking and iteration sane.
Hmmm, this initial implementation assumed that there is no hot pluggable HSI controllers, they are part of the SoC. Once the device tree is populated hsi clients, port and controllers devices will not disappear unless the hsi_controller driver is removed, which can not happen if there is still hsi_client drivers using it.
Quoted 6 lines
Quoted 2 lines
But I guess I can go back to the old code and push the issue down to the the hsi controller drivers and force them to use threaded interrupts...
I don't understand what you are trying to solve here. Any pointers to where I should look?
https://lkml.org/lkml/2011/6/10/275
Basically the ssi_wake_tasklet and related code. This code takes care of signal the clients that the other end is rising/dropping the HW wakeline because wants to transmit. Usually, hsi_clients should not care about this low level issues, but there is a race in HW protocol specification, and the usual solution is to have fix on the hsi_client for that by agreeing on a handshake.
Quoted 3 lines
thanks,
greg k-h
Br,
Carlos Chinea