lkml.org 
[lkml]   [2018]   [Apr]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v2 2/2] mailbox: add STMicroelectronics STM32 IPCC driver
    On Fri, Apr 6, 2018 at 8:35 PM, Fabien DESSENNE <fabien.dessenne@st.com> wrote:
    >
    > On 06/04/18 14:56, Jassi Brar wrote:
    >> On Fri, Apr 6, 2018 at 5:59 PM, Fabien DESSENNE <fabien.dessenne@st.com> wrote:
    >>> Hi
    >>>
    >>>
    >>> On 05/04/18 11:38, Jassi Brar wrote:
    >>>> On Mon, Mar 12, 2018 at 4:28 PM, Fabien Dessenne <fabien.dessenne@st.com> wrote:
    >>>> ....
    >>>>> +
    >>>>> + /* irq */
    >>>>> + for (i = 0; i < IPCC_IRQ_NUM; i++) {
    >>>>> + ipcc->irqs[i] = of_irq_get_byname(dev->of_node, irq_name[i]);
    >>>>> + if (ipcc->irqs[i] < 0) {
    >>>>> + dev_err(dev, "no IRQ specified %s\n", irq_name[i]);
    >>>>> + ret = ipcc->irqs[i];
    >>>>> + goto err_clk;
    >>>>> + }
    >>>>> +
    >>>>> + ret = devm_request_threaded_irq(dev, ipcc->irqs[i], NULL,
    >>>>> + irq_thread[i], IRQF_ONESHOT,
    >>>>> + dev_name(dev), ipcc);
    >>>>>
    >>>> In your interrupt handlers you don't do anything that could block.
    >>>> Threads only adds some delay to your message handling.
    >>>> So maybe use devm_request_irq() ?
    >>> The interrupt handlers call mbox_chan_received_data() /
    >>> mbox_chan_txdone(), which call in turn client's rx_callback() /
    >>> tx_done() / tx_prepare() which behavior may be unsafe. Hence, using a
    >>> threaded irq here seems to be a good choice.
    >>>
    >> rx_callback() is supposed to be atomic.
    >
    > I am worried with this atomic part (and honestly I did not note that the
    > callbacks were expected to be)
    >
    > In my case, remoteproc->virtio->rpmsg is the mailbox client defining the
    > rx_callback.
    > If I follow your suggestion, I shall make this rx_callback Atomic in
    > remoteproc (or in virtio or rpmsg). And this does not seem to be so
    > simple (add a worker in the middle of somewhere?). Bjorn, feel free to
    > comment this part.
    >
    > An alternate implementation consists in using a threaded IRQ for the
    > mailbox interrupt.
    > This option is not only simple, but also ensures to split bottom & half
    > parts at the irq level which is IMHO a general good practice.
    >
    > I can see that some mailbox clients implement callbacks that are NOT
    > atomic and I suspect this is the reason why some mailbox drivers use
    > threaded_irq (rockchip mailbox splits the bottom & half parts).
    >
    > Would it be acceptable to consider the "atomic client callback" as a
    > non-strict rule ?
    >
    Of course you can traverse atomic path from sleepable context (but not
    vice-versa).
    Please send in the final revision.

    Thanks.

    \
     
     \ /
      Last update: 2018-04-06 18:21    [W:2.984 / U:0.156 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site