lkml.org 
[lkml]   [2008]   [Dec]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [w1] Allow master IO commands.
On Thu, 18 Dec 2008 01:33:52 +0300
Evgeniy Polyakov <zbr@ioremap.net> wrote:

> Hi Andrew.
>
> On Wed, Dec 17, 2008 at 02:22:41PM -0800, Andrew Morton (akpm@linux-foundation.org) wrote:
> > > +static int w1_process_command_master(struct w1_master *dev, struct cn_msg *req_msg,
> > > + struct w1_netlink_msg *req_hdr, struct w1_netlink_cmd *req_cmd)
> > > +{
> > > + int err = -EINVAL;
> > > + struct cn_msg *msg;
> > > + struct w1_netlink_msg *hdr;
> > > + struct w1_netlink_cmd *cmd;
> > > +
> > > + msg = kzalloc(PAGE_SIZE, GFP_KERNEL);
> >
> > The use of PAGE_SIZE is odd. It's either too large (inefficient) or
> > too small (disaster).
> >
> > Is it not practical to calculate this precisely?
>
> It is not possible to calculate it in advance for the search commands,
> since we do not know how many slave devices are attached to the bus.
> This buffer is used as temporal area where IDs are stored and when
> number of IDs is large enough message is emmitted to the userspace and
> buffer becomes empty again. Sending one message per slave ID is a too
> big overhead, so this could be half of a page or one third or whatever
> else, I selected a page, which is the maximum guaranteed allocation
> buffer size.

OK.

Do we have checks in there to ensure that we can't run off the end of
the buffer?

> > > + if (!msg)
> > > + return -ENOMEM;
> > > +
> > > + msg->id = req_msg->id;
> > > + msg->seq = req_msg->seq;
> > > + msg->ack = 0;
> > > + msg->len = sizeof(struct w1_netlink_msg) + sizeof(struct w1_netlink_cmd);
> > > +
> > > + hdr = (struct w1_netlink_msg *)(msg + 1);
> > > + cmd = (struct w1_netlink_cmd *)(hdr + 1);
> > > +
> > > + hdr->type = W1_MASTER_CMD;
> > > + hdr->id = req_hdr->id;
> > > + hdr->len = sizeof(struct w1_netlink_cmd);
> > > +
> > > + cmd->cmd = req_cmd->cmd;
> > > + cmd->len = 0;
> > > +
> > > + switch (cmd->cmd) {
> > > + case W1_CMD_SEARCH:
> >
> > checkpatch correctly points out that the body of the switch statement
> > should be indented one tabstop less than this.
> >
> > > + case W1_CMD_ALARM_SEARCH:
> > > + err = w1_process_search_command(dev, msg,
> > > + PAGE_SIZE - msg->len - sizeof(struct cn_msg));
> >
> > which would give more room for cleaning this up.
>
> Ok, I will space it to the left. Should I sent patch on top of it or
> instead of?

Is OK, I'll fix it. That change messes up the following patches so I
fixed them as well.



\
 
 \ /
  Last update: 2008-12-18 00:17    [W:1.232 / U:0.068 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site