On Fri, Feb 14, 2014 at 08:48:25PM +0100, Arnd Bergmann wrote: > On Wednesday 12 February 2014, Courtney Cavin wrote: > > On Tue, Feb 11, 2014 at 09:35:01AM +0100, Arnd Bergmann wrote: > > > On Monday 10 February 2014 16:23:48 Courtney Cavin wrote: > > > Then again, I think that the context management stuff is the exception as well, > > and I think that can/should also be handled in a higher level. Regardless, I > > went ahead and drafted the async flags idea out anyway, so here's some > > pseudo-code. I also tried to shoe-horn in 'peek', and you can see how that > > turns out. Let me know if this is something like what you had in mind. > > The async implementation looks good to me, assuming we actually need both > sync and async operations, which I can't tell for sure. Yea, I would like some further input on that specifically. I have added Linus Walleij and Jassi Brar, who have had good input on mailboxes in the past, and somehow I missed in this series. > For the peek operation, it wouldn't work for the ethernet case, which > has to call it from atomic context in net_rx_action. It wouldn't work if the mbox is not requested with MBOX_ASYNC, but otherwise that should be fine, as it would just peek into the kfifo. That doesn't seem like a desirable method for ethernet use-case though, as it ends up being two extra copies. > > /** > > * so this is where this lock makes things difficult, as this function > > * might_sleep(), but only really because of the lock. Either we can > > * remove the lock and force the adapter to do its own locking > > * spinlock-style, or we can accept the sleep here, which seems a bit > > * stupid in a peek function. Neither option is good. Additionally, > > * there's no guarantee that the adapter doesn't operate over a bus > > * which itself might_sleep(), exacerbating the problem. > > */ > > mutex_lock(&mbox->adapter->lock); > > rc = mbox->adapter->ops->peek_message(mbox->adapter, mbox->chan, msg); > > mutex_lock(&mbox->adapter->lock); > > If we decide that peek() must not sleep, any driver that operates on a > slow bus could just always report "no data" here. Yes indeed, or it could just not implement peek, which seems reasonable. > Moving the locking into the mbox driver here sounds appropriate. I don't really like doing that for the entirety of the mbox core, as it makes the simple adapters harder to write properly. Since peek is not a typical use-case, perhaps we could remove the locking for just peek, and have a Big Fat Warning in the description of how to properly implement it? > Arnd Thanks for the input! -Courtney -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
