Re: [w1] Allow master IO commands.

[Evgeniy Polyakov]

Hi Andrew.

On Wed, Dec 17, 2008 at 02:22:41PM -0800, Andrew Morton (akpm@xxxxxxxxxxxxxxxxxxxx) 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.

> > +	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?

-- 
	Evgeniy Polyakov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/