| 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
| ||
|
Message-Id: <1198801142.5354.67.camel@odie>
Date: Fri, 28 Dec 2007 01:18:57 +0100
From: Simon Holm Thøgersen <odie@...aau.dk>
To: Joe Perches <joe@...ches.com>
Cc: Adrian McMenamin <adrian@...golddream.dyndns.info>,
Paul Mundt <lethal@...ux-sh.org>,
Jens Axboe <jens.axboe@...cle.com>,
LKML <linux-kernel@...r.kernel.org>,
linux-sh <linux-sh@...r.kernel.org>
Subject: Re: [PATCH] SH/Dreamcast - add support for GD-Rom device
tor, 27 12 2007 kl. 14:58 -0800, skrev Joe Perches:
> On Thu, 2007-12-27 at 16:52 +0000, Adrian McMenamin wrote:
> > This patch adds support for the CD-Rom drive on the SEGA Dreamcast.
>
> Because it was already so close, might as well make it checkpatch clean.
>
> I also added a function gdrom_is_busy() to make a couple of tests
> fit on a single line and perhaps easier to read.
>
> Signed-off-by: Joe Perches <joe@...ches.com>
>
> --- a/drivers/cdrom/gdrom.c 2007-12-27 14:49:27.000000000 -0800
> +++ b/drivers/cdrom/gdrom.c 2007-12-27 14:46:20.000000000 -0800
> @@ -86,7 +86,8 @@
> {MEDIUM_ERROR, "GDROM: Disk not ready"},
> {HARDWARE_ERROR, "GDROM: Hardware error"},
> {ILLEGAL_REQUEST, "GDROM: Command has failed"},
> - {UNIT_ATTENTION, "GDROM: Device needs attention - disk may have been changed"},
> + {UNIT_ATTENTION, "GDROM: Device needs attention - "
> + "disk may have been changed"},
> {DATA_PROTECT, "GDROM: Data protection error"},
> {ABORTED_COMMAND, "GDROM: Command aborted"},
> };
> @@ -130,14 +131,20 @@
> };
>
> static int gdrom_getsense(short *bufstring);
> -static int gdrom_packetcommand(struct cdrom_device_info *cd_info, struct packet_command *command);
> +static int gdrom_packetcommand(struct cdrom_device_info *cd_info,
> + struct packet_command *command);
> static int gdrom_hardreset(struct cdrom_device_info *cd_info);
>
> +static bool gdrom_is_busy(void)
> +{
> + return (ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) != 0;
> +}
> +
> static void gdrom_wait_clrbusy(void)
> {
> /* long timeouts - typical for a CD Rom */
> unsigned long timeout = jiffies + HZ * 60;
> - while ((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) && (time_before(jiffies, timeout)))
> + while (gdrom_is_busy() && (time_before(jiffies, timeout)))
^ ^
You don't need those parentheses.
> cpu_relax();
> }
>
> @@ -146,7 +153,7 @@
> unsigned long timeout;
> /* Wait to get busy first */
> timeout = jiffies + HZ * 60;
> - while (((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) == 0) && (time_before(jiffies, timeout)))
> + while (!gdrom_is_busy() && (time_before(jiffies, timeout)))
Same here.
> cpu_relax();
> /* Now wait for busy to clear */
> gdrom_wait_clrbusy();
> @@ -216,7 +223,8 @@
> gd.pending = 1;
> gdrom_packetcommand(gd.cd_info, spin_command);
> /* 60 second timeout */
> - wait_event_interruptible_timeout(command_queue, gd.pending == 0, HZ * 60);
> + wait_event_interruptible_timeout(command_queue,
> + gd.pending == 0, HZ * 60);
> gd.pending = 0;
All three users of gdrom_packetcommand do the same timeout, so consider
moving this block into gdrom_packcommand.
> kfree(spin_command);
> if (gd.status & 0x01) {
> @@ -249,7 +257,8 @@
> toc_command->buflen = tocsize;
> gd.pending = 1;
> gdrom_packetcommand(gd.cd_info, toc_command);
> - wait_event_interruptible_timeout(command_queue, gd.pending == 0, HZ * 60);
> + wait_event_interruptible_timeout(command_queue,
> + gd.pending == 0, HZ * 60);
> gd.pending = 0;
> insw(PHYSADDR(GDROM_DATA_REG), toc, tocsize/2);
> kfree(toc_command);
> @@ -274,7 +283,8 @@
> return (track & 0x0000ff00) >> 8;
> }
>
> -static int gdrom_get_last_session(struct cdrom_device_info *cd_info, struct cdrom_multisession *ms_info)
> +static int gdrom_get_last_session(struct cdrom_device_info *cd_info,
> + struct cdrom_multisession *ms_info)
> {
> int fentry, lentry, track, data, tocuse, err;
> if (!gd.toc)
> @@ -287,7 +297,8 @@
> tocuse = 0;
> err = gdrom_readtoc_cmd(gd.toc, 0);
> if (err) {
> - printk(KERN_INFO "GDROM: Could not get CD table of contents\n");
> + printk(KERN_INFO "GDROM: Could not get CD "
> + "table of contents\n");
> return -ENXIO;
> }
> }
> @@ -305,7 +316,8 @@
>
> if ((track > 100) || (track < get_entry_track(gd.toc->first))) {
> gdrom_getsense(NULL);
> - printk(KERN_INFO "GDROM: No data on the last session of the CD\n");
> + printk(KERN_INFO "GDROM: No data on the last session "
> + "of the CD\n");
> return -ENXIO;
> }
>
> @@ -355,8 +367,10 @@
> return 0;
> }
>
> -/* keep the function looking like the universal CD Rom specification - returning int*/
> -static int gdrom_packetcommand(struct cdrom_device_info *cd_info, struct packet_command *command)
> +/* keep the function looking like the universal CD Rom specification -
> + * returning int */
> +static int gdrom_packetcommand(struct cdrom_device_info *cd_info,
> + struct packet_command *command)
> {
> gdrom_spicommand(&command->cmd, command->buflen);
> return 0;
> @@ -388,7 +402,8 @@
> gd.pending = 1;
> gdrom_packetcommand(gd.cd_info, sense_command);
> /* 60 second timeout */
> - wait_event_interruptible_timeout(command_queue, gd.pending == 0, HZ * 60);
> + wait_event_interruptible_timeout(command_queue,
> + gd.pending == 0, HZ * 60);
> gd.pending = 0;
> kfree(sense_command);
> insw(PHYSADDR(GDROM_DATA_REG), &sense, 5);
> @@ -400,7 +415,8 @@
> printk(KERN_INFO "%s\n", sense_texts[sense_key].text);
>
> if (bufstring)
> - memcpy(bufstring, &sense[4], 2); /* return additional sense data */
> + memcpy(bufstring, &sense[4], 2);
> + /* return additional sense data */
Put the comment on the if-line or use curly brackets and give the
comment its own line above memcpy?
>
> if (sense_key < 2)
> return 0;
> @@ -434,7 +450,8 @@
> return cdrom_media_changed(gd.cd_info);
> }
>
> -static int gdrom_bdops_ioctl(struct inode *inode, struct file *file, unsigned cmd, unsigned long arg)
> +static int gdrom_bdops_ioctl(struct inode *inode, struct file *file,
> + unsigned cmd, unsigned long arg)
> {
> return cdrom_ioctl(file, gd.cd_info, inode, cmd, arg);
> }
> @@ -471,11 +488,13 @@
> {
> int err;
> init_waitqueue_head(&command_queue);
> - err = request_irq(HW_EVENT_GDROM_CMD, gdrom_command_interrupt, IRQF_DISABLED, "gdrom_command", &gd);
> + err = request_irq(HW_EVENT_GDROM_CMD, gdrom_command_interrupt,
> + IRQF_DISABLED, "gdrom_command", &gd);
> if (err)
> return err;
> init_waitqueue_head(&request_queue);
> - err = request_irq(HW_EVENT_GDROM_DMA, gdrom_dma_interrupt, IRQF_DISABLED, "gdrom_dma", &gd);
> + err = request_irq(HW_EVENT_GDROM_DMA, gdrom_dma_interrupt,
> + IRQF_DISABLED, "gdrom_dma", &gd);
> if (err)
> free_irq(HW_EVENT_GDROM_CMD, &gd);
> return err;
> @@ -531,27 +550,30 @@
> ctrl_outb(0, GDROM_INTSEC_REG);
> /* In multiple DMA transfers need to wait */
> timeout = jiffies + HZ / 2;
> - while ((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x80) && (time_before(jiffies, timeout)))
> + while (gdrom_is_busy() && (time_before(jiffies, timeout)))
> cpu_relax();
Another one here.
> ctrl_outb(GDROM_COM_PACKET, GDROM_STATUSCOMMAND_REG);
> timeout = jiffies + HZ / 2;
> - while (((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) != 8) && (time_before(jiffies, timeout)))
> + while (((ctrl_inb(GDROM_ALTSTATUS_REG) & 0x88) != 8) &&
> + (time_before(jiffies, timeout)))
What about a nice telling function like gdrom_is_busy for this?
> cpu_relax(); /* wait for DRQ to be set to 1 */
> gd.pending = 1;
> gd.transfer = 1;
> outsw(PHYSADDR(GDROM_DATA_REG), &read_command->cmd, 6);
> timeout = jiffies + HZ / 2;
> - while ((ctrl_inb(GDROM_DMA_STATUS_REG)) && (time_before(jiffies, timeout)))
> + while ((ctrl_inb(GDROM_DMA_STATUS_REG)) &&
> + (time_before(jiffies, timeout)))
Extra parentheses, also around ctrl_inb. And there should be curly
brackets around the following line, since the while condition is on two
lines.
> cpu_relax();
> ctrl_outb(1, GDROM_DMA_STATUS_REG);
> /* 5 second error margin here seems more reasonable */
> - wait_event_interruptible_timeout(request_queue, gd.transfer == 0, HZ * 5);
> + wait_event_interruptible_timeout(request_queue,
> + gd.transfer == 0, HZ * 5);
> err = ctrl_inb(GDROM_DMA_STATUS_REG);
> err = gd.transfer;
> gd.transfer = 0;
> gd.pending = 0;
> /* now seek to take the request spinlock
> - * before handling ending the request */
> + * before handling ending the request */
> spin_lock(&gdrom_lock);
> list_del_init(&req->queuelist);
> blk_requeue_request(gd.gdrom_rq, req);
> @@ -567,7 +589,7 @@
> static void gdrom_request_handler_dma(struct request *req)
> {
> /* dequeue, add to list of deferred work
> - * and then schedule workqueue */
> + * and then schedule workqueue */
> blkdev_dequeue_request(req);
> list_add_tail(&req->queuelist, &gdrom_deferred);
> schedule_work(&work);
> @@ -582,7 +604,8 @@
> end_request(req, 0);
> }
> if (rq_data_dir(req) != READ) {
> - printk(KERN_NOTICE "GDROM: Read only device - write request ignored\n");
> + printk(KERN_NOTICE "GDROM: Read only device - "
> + "write request ignored\n");
> end_request(req, 0);
> }
> if (req->nr_sectors)
> @@ -612,7 +635,8 @@
> firmw_ver = kstrndup(id->firmver, 16, GFP_KERNEL);
> if (!firmw_ver)
> goto free_manuf_name;
> - printk(KERN_INFO "GDROM: %s from %s with firmware %s\n", model_name, manuf_name, firmw_ver);
> + printk(KERN_INFO "GDROM: %s from %s with firmware %s\n",
> + model_name, manuf_name, firmw_ver);
> err = 0;
> kfree(firmw_ver);
> free_manuf_name:
> @@ -641,7 +665,8 @@
> gd.cd_info->ops = &gdrom_ops;
> gd.cd_info->capacity = 1;
> strcpy(gd.cd_info->name, GDROM_DEV_NAME);
> - gd.cd_info->mask = CDC_CLOSE_TRAY|CDC_OPEN_TRAY|CDC_LOCK|CDC_SELECT_DISC;
> + gd.cd_info->mask = CDC_CLOSE_TRAY | CDC_OPEN_TRAY |
> + CDC_LOCK | CDC_SELECT_DISC;
> }
>
> static void __devinit probe_gdrom_setupdisk(void)
> @@ -671,7 +696,7 @@
> {
> int err;
> if (gdrom_execute_diagnostic() != 1) {
> - printk(KERN_WARNING "GDROM: ATA Probe for GDROM failed.\n");
> + printk(KERN_WARNING "GDROM: ATA Probe for GDROM failed\n");
> return -ENODEV;
> }
> if (gdrom_outputversion())
> @@ -679,7 +704,8 @@
> gdrom_major = register_blkdev(0, GDROM_DEV_NAME);
> if (gdrom_major <= 0)
> return gdrom_major;
> - printk(KERN_INFO "GDROM: Block device is registered with major number %d\n", gdrom_major);
> + printk(KERN_INFO "GDROM: Block device is registered "
> + "with major number %d\n", gdrom_major);
> gd.cd_info = kzalloc(sizeof(struct cdrom_device_info), GFP_KERNEL);
> if (!gd.cd_info) {
> err = -ENOMEM;
> @@ -724,7 +750,8 @@
> unregister_blkdev(gdrom_major, GDROM_DEV_NAME);
> gdrom_major = 0;
> probe_fail_no_mem:
> - printk(KERN_WARNING "GDROM: Could not probe for device - error is 0x%X\n", err);
> + printk(KERN_WARNING "GDROM: Could not probe for device - "
> + "error is 0x%X\n", err);
> return err;
> }
Simon Holm Thøgersen
--
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