Re: [PATCH v5 2/2] media: rc: introduce Meson IR TX driver

[Sean Young <sean@mess.org>]

Hi Viktor,

On Fri, Jul 16, 2021 at 01:36:52AM +0300, Viktor Prutyanov wrote:
> Hi Sean,
> 
> On Thu, 15 Jul 2021 22:40:01 +0100
> Sean Young <sean@mess.org> wrote:
> 
> > On Thu, Jul 15, 2021 at 12:27:06AM +0300, Viktor Prutyanov wrote:
 > > +	meson_irtx_fill_buf(ir, tx_buf, buf, len);
> > > +	dev_dbg(ir->dev, "TX buffer filled, length = %u\n", len);
> > > +
> > > +	spin_lock_irqsave(&ir->lock, flags);
> > > +	meson_irtx_update_buf(ir, tx_buf, len, 0);
> > > +	reinit_completion(&ir->completion);
> > > +	meson_irtx_send_buffer(ir);
> > > +	spin_unlock_irqrestore(&ir->lock, flags);
> > > +
> > > +	ret = wait_for_completion_interruptible(&ir->completion);
> > > +	dev_dbg(ir->dev, "TX %s\n", ret ? "interrupted" :
> > > "completed");  
> > 
> > Here two things can happen. One is, the process received a signal
> > (e.g. ^C). The other is that the hardware didn't issue any interrupts
> > due some problem somewhere. In the latter case, we only escape this
> > wait_for_completion_interruptable() when the user gets fed up and
> > presses ^C or something like that.
> > 
> > > +
> > > +	spin_lock_irqsave(&ir->lock, flags);
> > > +	kfree(ir->buf);
> > > +	meson_irtx_update_buf(ir, NULL, 0, 0);
> > > +	spin_unlock_irqrestore(&ir->lock, flags);  
> > 
> > Now it is possible that the buffer gets cleared before that IR was
> > sent, if the signal was received early enough. This means not all the
> > Tx was completed.
> > 
> > > +
> > > +	return len;  
> > 
> > Yet, we always return success.
> > 
> > In case no interrupts were generated we should return an error in a
> > timely manner, so the wait_for_completion() needs the timeout. You
> > can use the fact that the IR is never longer IR_MAX_DURATION (half a
> > second currently). Not sure what the returned error should be, maybe
> > -ETIMEDOUT?
> 
> As for me, ETIMEDOUT is OK.
> > 
> > The problem with the interruptable wait is that a process can receive
> > a signal at any time, and now when this happens your IR gets
> > truncated. I don't think this is what you want.
> 
> Should I replace wait_for_completion_interruptible by
> wait_for_completion_timeout in order to wait in uninterruptible way?

Yes, the process can receive a signal if the terminal is resized
(SIGWINCH) or if the process is backgrounded and then foregrounded with
^Z and fg (SIGCONT). If this happens during tx then the tx might be
incomplete.

Thanks,

Sean