| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 3 May 2007 02:37:21 -0600
From: "Grant Likely" <grant.likely@...retlab.ca>
To: "Andrew Morton" <akpm@...ux-foundation.org>
Cc: "Jens Axboe" <axboe@...nel.dk>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Add support for Xilinx SystemACE CompactFlash interface.
On 5/2/07, Andrew Morton <akpm@...ux-foundation.org> wrote:
> On Wed, 2 May 2007 00:43:16 -0600 Grant Likely <grant.likely@...retlab.ca> wrote:
> > + * To Do:
> > + * - Add FPGA configuration control interface.
> > + * - Request major number from lanana
> > + * - Add legacy device geometry ioctl
> > + */
>
> Swoon. Want a job writing kernel comments?
Heh; I don't know, how much does that pain pay? :-)
> >
> > +static LIST_HEAD(ace_instances);
> > +static int ace_major = 0;
>
> Pleas eremove the "= 0;': it takes up space in vmlinux, but .bss is zeroed
> anyway.
Oops; yes. I'm waiting on a major number assignment from lanana.
It's supposed to go here.
> > + void (*out)(struct ace_device *ace, ulong reg, uint16_t val);
> > + void (*identin)(struct ace_device *ace);
> > + void (*datain)(struct ace_device *ace);
> > + void (*dataout)(struct ace_device *ace);
> > +};
> > +
> > +/* 8 Bit bus width */
> > +static uint16_t ace_in_8(struct ace_device *ace, ulong reg)
>
> Here we have the correct `foo *bar';
>
> > +{
> > + void* r = ace->baseaddr + reg;
>
> And here we have the kernelly-incorrect `foo* bar;'
>
> Please do s/* / */ everywhere.
/me runs entire driver through scripts/Lindent for good measure.
> > +static void ace_identin_8(struct ace_device *ace)
> > +{
> > + void* r = ace->baseaddr + 0x40;
> > + int i = ACE_FIFO_SIZE/2;
> > + while (i--)
> > +#if defined(__BIG_ENDIAN)
> > + *ace->data_ptr++ = (in_8(r)) | (in_8(r+1)<<8);
> > +#else
> > + *ace->data_ptr++ = (in_8(r)<<8) | (in_8(r+1));
> > +#endif
> > +}
>
> This ifdeffery appears in several places. SUggest the addition of a helper
> function which does it in a single place.
It's actually two different things that look very similar, and there
are only 4 tests for __BIG_ENDIAN in the whole driver, and they're all
different. But the point is moot; I messed up the 8 bit accessors.
It will be reimplemented in the next patch.
> > +{
> > +#ifndef __LITTLE_ENDIAN
> > +# ifdef __BIG_ENDIAN
> > + /* The ace_reg_read16 macro handles 16 bit reads correctly, but
> > + * 32bit values are partially little endian; swap the words
> > + */
> > + id->lba_capacity = ((id->lba_capacity >> 16) & 0x0000FFFF) |
> > + ((id->lba_capacity << 16) & 0xFFFF0000);
> > + id->spg = ((id->spg >> 16) & 0x0000FFFF) |
> > + ((id->spg << 16) & 0xFFFF0000);
> > +# else
> > +# error "Please fix <asm/byteorder.h>"
>
> Don't you trust us? :)
That's what happens when I copy a snippit from another parts of the kernel. :-P
>
> > +#if defined(DEBUG)
> > +const char* ace_statenames[ACE_FSM_NUM_STATES] = {
> > + "idle",
> > + "req lock",
> > + "wait lock",
> > + "wait cf ready",
> > + "identify prepare",
> > + "identify transfer",
> > + "identify complete",
> > + "request prepare",
> > + "request transfer",
> > + "request complete",
> > +};
> > +#endif
>
> Can these have static storage class?
Yeah, but I'll just remove them instead
> > +static int ace_release(struct inode *inode, struct file *filp)
> > +{
> > + struct ace_device *ace = inode->i_bdev->bd_disk->private_data;
> > + unsigned long flags;
> > + uint16_t val;
> > +
> > + ace_dbg(ace, "ace_release() users=%i\n", ace->users-1);
> > +
> > + spin_lock_irqsave(&ace->lock, flags);
> > + ace->users--;
> > + if (ace->users == 0) {
> > + val = ace_reg_read16(ace, ACE_CTRL);
> > + ace_reg_write16(ace, ACE_CTRL, val & ~ACE_CTRL_LOCKREQ);
> > + }
> > + spin_unlock_irqrestore(&ace->lock, flags);
> > + return 0;
> > +}
>
> Who owns gendisk.private_data? Is it the device driver? I've never
> looked...
gd.private_data points to the struct ace_device; which is freed in ace_remove().
>
> > +static int ace_ioctl(struct inode *inode, struct file *filp,
> > + unsigned int cmd, unsigned long arg)
> > +{
> > + struct ace_device *ace = inode->i_bdev->bd_disk->private_data;
> > + ace_dbg(ace, "ace_ioctl()\n");
> > +
> > + return -ENOTTY;
> > +}
>
> I suspect you can just leave this unimplemented.
But keeping it in reminds me that I need to get it done. :-) It will
be implemented in the next patch.
> > + /*
> > + * Initialize the request queue
> > + */
> > + ace->queue = blk_init_queue(ace_request, &ace->lock);
> > + if (ace->queue == NULL)
> > + goto err_blk_initq;
> > + blk_queue_hardsect_size(ace->queue, 512);
> > +
> > + /*
> > + * Allocate and initialize GD structure
> > + */
> > + ace->gd = alloc_disk(ACE_NUM_MINORS);
> > + if (!ace->gd)
> > + goto err_alloc_disk;
> > +
> > + ace->gd->major = ace_major;
> > + ace->gd->first_minor = ace->id * ACE_NUM_MINORS;
> > + ace->gd->fops = &ace_fops;
> > + ace->gd->queue = ace->queue;
> > + ace->gd->private_data = ace;
> > + snprintf(ace->gd->disk_name, 32, "xs%c", ace->id + 'a');
> > + device_rename(ace->dev, ace->gd->disk_name);
>
> Now why are we doing a device_rename() here? The only other caller in the
> tree is the netdev renaming code. I suspect something is awry here.
Mostly so that the device shows up as '/sys/block/xsa' instead of
'/sys/block/xsysace.0'. Plus it shows 'xsa' in the log when using
dev_dbg() macros.
I can remove this if this is a bad idea.
>
> Looks nice though.
Thanks for the comments; I'll clean up and resubmit.
Cheers,
g.
--
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@...retlab.ca
(403) 399-0195
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists