>-----Original Message-----
>From: Arjan van de Ven [mailto:[email protected]]
>Sent: Saturday, June 24, 2006 4:00 AM
>To: [email protected]
>Cc: Randy.Dunlap; [email protected]; Gross, Mark
>Subject: Re: [PATCH] riport LADAR driver
>
>
>since this is for a tutorial... double nitpick mode ;-)
>(since examples should be squeeky clean or people will turn the right
>thing into the not quite right thing later in their own code)
Way cool. Thanks! I'll update the patch and respond to your issues later
today.
>
>> +#undef PDEBUG
>> +#ifdef RIPORT_DEBUG
>> +# define PDEBUG(fmt, args...) printk( KERN_DEBUG "riport: " fmt, ##
args)
>> +#else /* */
>> +# define PDEBUG(fmt, args...)
>> +#endif /* */
>
>this is still there while it really shouldn't be; either use pr_debug()
>or dev_printk().
>
OK. I had a dumb reason in my head for not making this change. (I had
it in my head the DEBUG define was more global than it was...)
>
>> +struct devriport {
>> + unsigned int io;
>> + unsigned int io_ext;
>> + int irq;
>> + int dma;
>> + int size; /* buffer size */
>> + unsigned char *pbuf; /* pointer to the start of the memory
that
>> + stores scans from the riegl */
>> + unsigned char *pbuf_top;/* pointer to the end of pbuf (see
above) */
>> + unsigned char *pin; /* pointer to the end of new data */
>> + unsigned char *pout; /* pointer to the start of new data (end
of
>> + old/read data) */
>> + wait_queue_head_t qwait;
>> + struct inode *pinode;
>> + struct file *pfile;
>> + int usage;
>> + int irqinuse;
>> + int readstate;
>> + short syncWord;
>> + int numbytesthisstate;
>> + int bytestoread;
>> + char buf[4];
>> + struct timeval timestamp;
>> +
>> + spinlock_t lock;
>> +};
>
>if this is for a tutorial.. might as well sort these fields in order of
>decreasing size so that you get minimal alignment packing by the
>compiler
Ok
>
>> + if (!request_region(io + ECP_OFFSET, 3, "riport")) {
>> + release_region(io,3);
>> +
>> + PDEBUG("request_region 0x%X of 3 bytes fails\n", io +
ECP_OFFSET );
>> + *presult = -EBUSY;
>> + goto fail_io;
>> + }
>
>might as well make another goto target and have that do the
>release_region...
OK,
>
>> +
>> +static int devriport_release(struct devriport *this)
>> +{
>> + this->irqinuse = 0;
>> +
>> + /* switch to compatibility mode */
>> + outb(ECR_SPP_MODE | ECR_ERRINT_DISABLED | ECR_SERVICE_INTERRUPT,
>> + this->io_ext + ECR_EXT);
>> + outb(DCR_NOT_REVERSE_REQUEST | DCR_SELECT_IN, this->io +
DCR_BASE);
>> +
>> + free_irq(this->irq, this);
>> + kfree(this->pbuf);
>> +
>> + this->usage--;
>> + WARN_ON(this->usage < 0);
>> + PDEBUG("release\n");
>> + return 0;
>> +}
>> +
>> +
>...
>
>> +
>> +
>> +static int devriport_open(struct devriport *this)
>> +{
>> + int result;
>> +
>> + if (this->usage)
>> + return -EBUSY;
>
>this "usage count" thing is probably buggy and racy; what is it for?
I'll look at this more closely. I bet we can loose it.
Thanks again,
The patch will come later this weekend.
--mgross
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
