Re: [PATCH] Altix system controller communication driver

[Greg Howard]

Hi Andrew,

On Wed, 28 Jul 2004, Andrew Morton wrote:

> As Jes says,
>
> 	.owner	= THIS_MODULE,
>
> is preferred here.
. . .
>
> There's no need to cast the return value of kmalloc.
>
> 	scd = kmalloc(sizeof(*scd), GFP_KERNEL);
>
> would suffice here.

I fixed these.  I plan to repost shortly.

. . .
> hm.  O_NONBLOCK means "don't wait for more input to arrive" rather than
> "don't block if someone else is holding a lock I want".  But given that the
> semaphore is held by !O_NONBLOCK readers, it has to be done this way.
>
> I guess there's no bug here, but it's a bit odd.

I'm certainly willing to try other ways, but this was the best I
could think of.  It seems to work at least.

>
> > +		copy_to_user(buf, sd->sd_rb, len);
>
> What Jes said: return -EFAULT if copy_to_user() returns non-zero.

Fixed this.

>
> > +static unsigned int
> > +scdrv_poll(struct file *file, struct poll_table_struct *wait)
> > +{
> > +	unsigned int mask = 0;
> > +	int status = 0;
> > +	struct subch_data_s *sd = (struct subch_data_s *) file->private_data;
> > +	unsigned long flags;
> > +
> > +	scdrv_lock_all(sd, &flags);
> > +	poll_wait(file, &sd->sd_rq, wait);
> > +	poll_wait(file, &sd->sd_wq, wait);
>
> This function will sleep with spinlocks held, won't it?

My understanding is that poll_wait just sets up a poll_table
structure; it doesn't sleep itself.

Thanks for the suggestions.
- Greg
-
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/