| Message ID | 20200623040107.22270-9-warthog618@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: cdev: add uAPI V2 | expand |
wt., 23 cze 2020 o 06:02 Kent Gibson <warthog618@gmail.com> napisał(a): > > Reset the timestamp field to 0 after using it in lineevent_irq_thread. > > The timestamp is set by lineevent_irq_handler and is tested by > lineevent_irq_thread to determine if it is called from a nested theaded > interrupt. > lineevent_irq_thread is assuming that the nested, or otherwise, status > of the IRQ is static, i.e. it is either always nested or never nested. > This change removes that assumption, resetting the timestamp so it can > be re-used to determine the nested state of subsequent interrupts. > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > This change makes sense to me but I'm having a hard time processing the explanation. If we're requesting the interrupt and allocating the lineevent state in the same function - how can we run into a situation here the status of the irq would change like what you describe? Bart
On Wed, Jun 24, 2020 at 04:00:42PM +0200, Bartosz Golaszewski wrote: > wt., 23 cze 2020 o 06:02 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > Reset the timestamp field to 0 after using it in lineevent_irq_thread. > > > > The timestamp is set by lineevent_irq_handler and is tested by > > lineevent_irq_thread to determine if it is called from a nested theaded > > interrupt. > > lineevent_irq_thread is assuming that the nested, or otherwise, status > > of the IRQ is static, i.e. it is either always nested or never nested. > > This change removes that assumption, resetting the timestamp so it can > > be re-used to determine the nested state of subsequent interrupts. > > > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > > > > This change makes sense to me but I'm having a hard time processing > the explanation. If we're requesting the interrupt and allocating the > lineevent state in the same function - how can we run into a situation > here the status of the irq would change like what you describe? > I'm not totally sure myself, as my understanding of how interrupts are shared in the kernel is pretty sketchy, but my concern is that if we are sharing the irq then whoever we are sharing with may release the irq and we go from nested to unnested. Or vice versa. Not sure if that is valid, but that was my concern, and it seemed like a minor change to cover it just in case. Cheers, Kent.
On Wed, Jun 24, 2020 at 4:08 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Wed, Jun 24, 2020 at 04:00:42PM +0200, Bartosz Golaszewski wrote: > > wt., 23 cze 2020 o 06:02 Kent Gibson <warthog618@gmail.com> napisał(a): > > > > > > Reset the timestamp field to 0 after using it in lineevent_irq_thread. > > > > > > The timestamp is set by lineevent_irq_handler and is tested by > > > lineevent_irq_thread to determine if it is called from a nested theaded > > > interrupt. > > > lineevent_irq_thread is assuming that the nested, or otherwise, status > > > of the IRQ is static, i.e. it is either always nested or never nested. > > > This change removes that assumption, resetting the timestamp so it can > > > be re-used to determine the nested state of subsequent interrupts. > > > > > > Signed-off-by: Kent Gibson <warthog618@gmail.com> > > > > > > > This change makes sense to me but I'm having a hard time processing > > the explanation. If we're requesting the interrupt and allocating the > > lineevent state in the same function - how can we run into a situation > > here the status of the irq would change like what you describe? > > > > I'm not totally sure myself, as my understanding of how interrupts are > shared in the kernel is pretty sketchy, but my concern is that if we > are sharing the irq then whoever we are sharing with may release the irq > and we go from nested to unnested. Or vice versa. Not sure if that is > valid, but that was my concern, and it seemed like a minor change to > cover it just in case. > It's my understanding that a shared interrupt must be explicitly requested as shared by all previous users or request_irq() will fail. In this case: we call request_threaded_irq() without the IRQF_SHARED flag so it's never a shared interrupt. Even if someone previously requested it as shared - our call will simply fail. I still think that resetting the timestamp is fine because it's not being set to 0 in hardirq context. We just need a different explanation. Bart
On Thu, Jun 25, 2020 at 11:44:30AM +0200, Bartosz Golaszewski wrote: > On Wed, Jun 24, 2020 at 4:08 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Wed, Jun 24, 2020 at 04:00:42PM +0200, Bartosz Golaszewski wrote: > > > wt., 23 cze 2020 o 06:02 Kent Gibson <warthog618@gmail.com> napisał(a): [ snip ] > > > > I'm not totally sure myself, as my understanding of how interrupts are > > shared in the kernel is pretty sketchy, but my concern is that if we > > are sharing the irq then whoever we are sharing with may release the irq > > and we go from nested to unnested. Or vice versa. Not sure if that is > > valid, but that was my concern, and it seemed like a minor change to > > cover it just in case. > > > > It's my understanding that a shared interrupt must be explicitly > requested as shared by all previous users or request_irq() will fail. > In this case: we call request_threaded_irq() without the IRQF_SHARED > flag so it's never a shared interrupt. Even if someone previously > requested it as shared - our call will simply fail. > OK. Is there a reason not to share the interrupt? > I still think that resetting the timestamp is fine because it's not > being set to 0 in hardirq context. We just need a different > explanation. > Or just drop it? Cheers, Kent.
On Thu, Jun 25, 2020 at 12:01 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Thu, Jun 25, 2020 at 11:44:30AM +0200, Bartosz Golaszewski wrote: > > On Wed, Jun 24, 2020 at 4:08 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > On Wed, Jun 24, 2020 at 04:00:42PM +0200, Bartosz Golaszewski wrote: > > > > wt., 23 cze 2020 o 06:02 Kent Gibson <warthog618@gmail.com> napisał(a): > [ snip ] > > > > > > I'm not totally sure myself, as my understanding of how interrupts are > > > shared in the kernel is pretty sketchy, but my concern is that if we > > > are sharing the irq then whoever we are sharing with may release the irq > > > and we go from nested to unnested. Or vice versa. Not sure if that is > > > valid, but that was my concern, and it seemed like a minor change to > > > cover it just in case. > > > > > > > It's my understanding that a shared interrupt must be explicitly > > requested as shared by all previous users or request_irq() will fail. > > In this case: we call request_threaded_irq() without the IRQF_SHARED > > flag so it's never a shared interrupt. Even if someone previously > > requested it as shared - our call will simply fail. > > > > OK. Is there a reason not to share the interrupt? > If nobody requested this yet, I'd say: let's not touch it. :) In theory, we check if the line state changed so we should be fine but in practice this sounds like a can of worms. That being said: I don't have a reason not to do it. Just a feeling. > > I still think that resetting the timestamp is fine because it's not > > being set to 0 in hardirq context. We just need a different > > explanation. > > > > Or just drop it? Yes, I think dropping this patch for now is fine. Bart
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index d50339ef6f05..1e8e0a0a9b51 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -559,6 +559,8 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p) else ge.timestamp = le->timestamp; + le->timestamp = 0; + if (le->eflags & GPIOEVENT_REQUEST_RISING_EDGE && le->eflags & GPIOEVENT_REQUEST_FALLING_EDGE) { int level = gpiod_get_value_cansleep(le->desc);
Reset the timestamp field to 0 after using it in lineevent_irq_thread. The timestamp is set by lineevent_irq_handler and is tested by lineevent_irq_thread to determine if it is called from a nested theaded interrupt. lineevent_irq_thread is assuming that the nested, or otherwise, status of the IRQ is static, i.e. it is either always nested or never nested. This change removes that assumption, resetting the timestamp so it can be re-used to determine the nested state of subsequent interrupts. Signed-off-by: Kent Gibson <warthog618@gmail.com> --- drivers/gpio/gpiolib-cdev.c | 2 ++ 1 file changed, 2 insertions(+)