From: Jassi Brar <jassisinghbrar@gmail.com> To: Samuel Holland <samuel@sholland.org> Cc: Mark Rutland <mark.rutland@arm.com>, Ondrej Jirman <megous@megous.com>, Devicetree List <devicetree@vger.kernel.org>, linux-sunxi@googlegroups.com, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Maxime Ripard <mripard@kernel.org>, Chen-Yu Tsai <wens@csie.org>, Rob Herring <robh+dt@kernel.org>, Philipp Zabel <p.zabel@pengutronix.de>, linux-arm-kernel <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH v6 2/6] mailbox: sun6i-msgbox: Add a new mailbox driver Date: Fri, 14 Feb 2020 22:47:28 -0600 Message-ID: <CABb+yY3T1cL+E6Y1tGb5cuKLSY5m_zi=VOx4AJzuX40TMOSQTw@mail.gmail.com> (raw) In-Reply-To: <89168ba0-a8ac-b433-3f93-412b22a9bc1a@sholland.org> On Fri, Feb 14, 2020 at 9:48 PM Samuel Holland <samuel@sholland.org> wrote: > > On 2/12/20 8:18 PM, Samuel Holland wrote: > > Jassi, > > > > On 2/12/20 8:02 PM, Jassi Brar wrote: > >> On Sun, Jan 12, 2020 at 11:18 PM Samuel Holland <samuel@sholland.org> wrote: > >>> > >>> +static int sun6i_msgbox_send_data(struct mbox_chan *chan, void *data) > >>> +{ > >>> + struct sun6i_msgbox *mbox = to_sun6i_msgbox(chan); > >>> + int n = channel_number(chan); > >>> + uint32_t msg = *(uint32_t *)data; > >>> + > >>> + /* Using a channel backwards gets the hardware into a bad state. */ > >>> + if (WARN_ON_ONCE(!(readl(mbox->regs + CTRL_REG(n)) & CTRL_TX(n)))) > >>> + return 0; > >>> + > >>> + /* We cannot post a new message if the FIFO is full. */ > >>> + if (readl(mbox->regs + FIFO_STAT_REG(n)) & FIFO_STAT_MASK) { > >>> + mbox_dbg(mbox, "Channel %d busy sending 0x%08x\n", n, msg); > >>> + return -EBUSY; > >>> + } > >>> + > >> This check should go into sun6i_msgbox_last_tx_done(). > >> send_data() assumes all is clear to send next packet. > > > > sun6i_msgbox_last_tx_done() already checks that the FIFO is completely empty (as > > the big comment explains). So this error could only be hit in the knows_txdone > > == true case, if the client pipelines multiple messages by calling > > mbox_client_txdone() before the message is actually removed from the FIFO. > > > > From the comments in mailbox_controller.h, this kind of usage looks to be > > unsupported. In that case, I could remove the check entirely. Does that sound right? > > After more thought, I would prefer to keep the check. It is fast/simple, and it > keeps the hardware from getting into an inconsistent state. Silently dropping > messages sounds like a poor quality of implementation. > If the FIFO becomes full after calling send_data(), then last_tx_done() should not only check remote's irq status but also check that the data has been consumed from the FIFO locally (hence the check becomes redundant in send_data). Otherwise the last_tx_done is incomplete. And you actually end up writing more code (error handling and resubmission instead of the api managing it all transparently) > send_data() is documented in mailbox_controller.h as returning EBUSY, > error is usually returned for s/w check (like mssg too big for fifo), not h/w events. > and I see multiple other mailbox controllers implementing the same or a similar check. > That it encourages next developer to repeat, is another reason to do it right this time. Otherwise, I can live with that check in send_data. > If > that is not the way you intend for the API to work, then please update the > comments in mailbox_controller.h. > Mailbox implementations follow no spec. There may be prudent need to return from send_data, but practically I haven't come across any(?) platform that can't do without the check in send_data. Cheers! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply index Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-01-13 5:18 [PATCH v6 0/6] Allwinner sun6i message box support Samuel Holland 2020-01-13 5:18 ` [PATCH v6 1/6] dt-bindings: mailbox: Add a sun6i message box binding Samuel Holland 2020-01-13 9:30 ` Maxime Ripard 2020-01-13 22:38 ` Rob Herring 2020-01-13 5:18 ` [PATCH v6 2/6] mailbox: sun6i-msgbox: Add a new mailbox driver Samuel Holland 2020-01-13 9:15 ` Philipp Zabel 2020-02-13 2:02 ` Jassi Brar 2020-02-13 2:18 ` Samuel Holland 2020-02-15 3:48 ` Samuel Holland 2020-02-15 4:47 ` Jassi Brar [this message] 2020-02-15 6:19 ` [PATCH] mailbox: sun6i-msgbox: Remove unneeded FIFO status check Samuel Holland 2020-01-13 5:18 ` [PATCH v6 3/6] ARM: dts: sunxi: a83t: Add msgbox node Samuel Holland 2020-01-13 5:18 ` [PATCH v6 4/6] ARM: dts: sunxi: h3/h5: " Samuel Holland 2020-01-13 5:18 ` [PATCH v6 5/6] arm64: dts: allwinner: a64: " Samuel Holland 2020-01-13 5:18 ` [PATCH v6 6/6] arm64: dts: allwinner: h6: " Samuel Holland 2020-02-13 1:43 ` [PATCH v6 0/6] Allwinner sun6i message box support Samuel Holland
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CABb+yY3T1cL+E6Y1tGb5cuKLSY5m_zi=VOx4AJzuX40TMOSQTw@mail.gmail.com' \ --to=jassisinghbrar@gmail.com \ --cc=devicetree@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-sunxi@googlegroups.com \ --cc=mark.rutland@arm.com \ --cc=megous@megous.com \ --cc=mripard@kernel.org \ --cc=p.zabel@pengutronix.de \ --cc=robh+dt@kernel.org \ --cc=samuel@sholland.org \ --cc=wens@csie.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Linux-ARM-Kernel Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \ linux-arm-kernel@lists.infradead.org public-inbox-index linux-arm-kernel Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel AGPL code for this site: git clone https://public-inbox.org/public-inbox.git