On Tue, Feb 11, 2014 at 09:35:01AM +0100, Arnd Bergmann wrote:
> On Monday 10 February 2014 16:23:48 Courtney Cavin wrote:
>
> > While I'm not sure the dislike of notifiers entirely justifies not using
> > them here, where they seem to make sense, I can understand that they
> > might not fully implement what we need to expose.
>
> I think we need to look at a few more examples of things that people
> want to with the framework to come up with a good interface. It's
> possible that we end up with multiple alternative notification
> methods, but it would be good to come up with one that works well
> for everyone.
>
> > Regarding handlers running in IRQ context: currently the API is designed
> > to do just that. From the use-cases I've found, most message handlers
> > simply queue something to happen at a later point. This is logical, as
> > the callback will be async, so you'll need to swap contexts or add locks
> > in your consumer anyway.
>
> Right. You may also have some handlers that need to run with extreme
> latency constraints, so we need at least the option of getting the
> callback from hardirq context, or possibly from softirq/tasklet
> as in the dmaengine case.
>
> > The dma engine is large and confused, so I'm not sure entirely which
> > part you are refering to. I've looked at having async completion going
> > both ways, but what I see every time is code complication in both the
> > adapter and in the consumers in the simple use-case. It doesn't really
> > make sense to make an API which makes things so generic that it becomes
> > hard to use. What I tried to follow here when designing the API was
> > what I saw in the actual implementations, not what was future-proof:
> > - Message receive callbacks may be called from IRQ context
> > - Message send implementations may sleep
>
> I can imagine cases where you want to send messages from softirq
> context, or from the same context in which you received the incoming
> mail, so it would be good to have the API flexible enough to deal
> with that. As a first step, always allowing send to sleep seems
> fine. Alternatively, we could have a 'sync' flag in the send
> API, to choose between "arrange this message to be sent out as
> soon as possible, but don't sleep" and "send this message and
> make sure it has arrived at the other end" as you do now.
Although I'm not sure there's currently a requirement for this, I can see that
this could be needed in the future.
> > What I can do is try to alleviate implementation efforts of future
> > requirements--as honestly, we can't really say exactly what they are--by
> > turning the messages into structs themselves, so at a later point flags,
> > ack callbacks, and rainbows can be added. I can then stop using
> > notifiers, and re-invent that functionality with a mbox_ prefix.
>
> I don't think there is a point in reimplementing notifiers under a
> different name. The question is rather how we want to deal with
> the 'multiple listener' case. If this case is the exception rather
> than the rule, I'd prefer making the callback API handle only
> single listeners and adding higher-level abstractions for the
> cases where we do need multiple listeners, but it really depends
> on what people need.
I wasn't actually planning on directly ripping-off notifiers under a new name.
Rather, as switching to message structs is probably a good idea anyway, I don't
think the notifier API properly represents that,... the void pointer is a bit
vague. It could be that it would turn out as a wrapper around notifiers. As
you said though, I do think this is the exception, so I'm fine with the single
callback idea, as long as Rob and Suman agree that this will be suitable for
their use-cases.
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.
enum {
MBOX_ASYNC = BIT(0),
};
struct mbox_adapter_ops {
...
int (*put_message)(struct mbox_adapter *, struct mbox_channel *,
const struct mbox_message *msg)
int (*peek_message)(struct mbox_adapter *, struct mbox_channel *,
struct mbox_message *msg)
};
struct mbox;
#define MBOX_FIFO_SIZE PAGE_SZ /* or not? */
struct mbox_channel {
...
unsigned long flags; /* MBOX_ASYNC, for now */
struct mbox *consumer;
struct kfifo_rec_ptr_1 rx_fifo;
/**
* probably should be allocated only if user needs to call
* mbox_put_message with MBOX_ASYNC, instead of statically.
*/
STRUCT_KFIFO_REC_1(MBOX_FIFO_SIZE) tx_fifo;
struct work_struct rx_work;
struct work_struct tx_work;
}
struct mbox {
struct mbox_channel *chan;
struct mbox_adapter *adapter;
void (*cb)(void *, struct mbox *, const struct mbox_message *);
void *priv;
};
struct mbox_message {
void *data;
unsigned int len;
};
static void rx_work(struct work_struct *work)
{
struct mbox_channel *chan;
struct mbox_message msg;
chan = container_of(work, struct mbox_channel, rx_work);
msg.len = kfifo_out(&chan->rx_fifo, msg.data);
chan->consumer->cb(chan->consumer, &msg);
}
/* called from adapter, typically in a IRQ handler */
int mbox_channel_notify(struct mbox_channel *chan,
const struct mbox_message *msg)
{
if (chan->flags & MBOX_ASYNC) {
kfifo_in(&chan->rx_fifo, msg->data, msg->len);
schedule_work(&chan->rx_work);
return 0;
}
/*
* consumer may not sleep here, as they did not specify this
* requirement in channel flags when requesting
*/
return chan->consumer->cb(chan->consumer->priv, chan->consumer, msg);
}
static void tx_work(struct work_struct *work)
{
struct mbox_channel *chan;
struct mbox_message msg;
char buf[PAGE_SZ]; /* should max size be specified by the adapter? */
chan = container_of(work, struct mbox_channel, tx_work);
msg.len = kfifo_out(&chan->tx_fifo, buf, sizeof(buf));
msg.data = buf;
mutex_lock(&chan->adapter->lock);
chan->adapter->ops->put_message(chan->adapter, chan, &msg);
mutex_unlock(&chan->adapter->lock);
}
/* called from consumer */
int mbox_put_message(struct mbox *mbox, const struct mbox_message *msg,
unsigned long flags)
{
int rc;
if (flags & MBOX_ASYNC) {
kfifo_in(&chan->tx_fifo, msg->data, msg->len);
schedule_work(&mbox->chan->tx_work);
return 0;
}
/**
* obviously, if not because of the lock, then because the adapter
* should wait for an ACK from it's controller if appropriate.
*/
might_sleep();
mutex_lock(&mbox->adapter->lock);
rc = mbox->adapter->ops->put_message(mbox->adapter, mbox->chan, msg);
mutex_unlock(&mbox->adapter->lock);
return rc;
}
/* called from consumer; illustrates the problem with peek */
int mbox_peek_message(struct mbox *mbox, struct mbox_message *msg)
{
int rc;
if (chan->flags & MBOX_ASYNC) {
msg->len = kfifo_out_peek(&mbox->chan->rx_fifo,
msg->data, msg->len);
return 0;
}
/**
* It is unlikely that most adapters are able to properly implement
* this, so we have to allow for that.
*/
if (!mbox->adapter->ops->peek_message)
return -EOPNOTSUPP;
/**
* 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);
return rc;
}
struct mbox *mbox_request(struct device *dev, const char *con_id,
void (*receive)(void *, struct mbox *, const struct
mbox_message *),
void *priv, unsigned long flags)
{
struct mbox_channel *chan;
struct mbox *mbox;
int rc;
...
chan->flags = flags;
if (flags & MBOX_ASYNC) {
rc = kfifo_alloc(&chan->rx_fifo, MBOX_FIFO_SIZE, GFP_KERNEL);
if (rc)
return rc;
INIT_WORK(&chan->rx_work, rx_work);
}
if (1 /* consumer plans on calling put_message with MBOX_ASYNC */) {
INIT_KFIFO(chan->tx_fifo);
INIT_WORK(&chan->tx_work, tx_work);
}
chan->consumer = mbox;
mbox->cb = receive;
mbox->priv = priv;
...
return mbox;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
