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: Wed, 2 May 2007 20:44:08 +0100 From: Christoph Hellwig <hch@...radead.org> To: Stefan Richter <stefanr@...6.in-berlin.de> Cc: linux-kernel@...r.kernel.org, Kristian H??gsberg <krh@...hat.com>, Linus Torvalds <torvalds@...ux-foundation.org>, Andrew Morton <akpm@...ux-foundation.org>, linux1394-devel <linux1394-devel@...ts.sourceforge.net>, Christoph Hellwig <hch@...radead.org> Subject: Re: [PATCH 5/6] firewire: SBP-2 highlevel driver > + sg = (struct scatterlist *)orb->cmd->request_buffer; > + count = dma_map_sg(device->card->device, sg, orb->cmd->use_sg, > + orb->cmd->sc_data_direction); you need to handle the error case (count == 0) > + /* Convert the scatterlist to an sbp2 page table. If any > + * scatterlist entries are too big for sbp2 we split the as we go. */ Please set the max_sectors value in your host template so that the block layer doesn't build sg entries too big for you. > + orb->page_table_bus = > + dma_map_single(device->card->device, orb->page_table, > + size, DMA_TO_DEVICE); This needs handling of mapping errors (dma_mapping_error()) > + orb = kzalloc(sizeof *orb, GFP_ATOMIC); Normal kernel style is sizeof(*orb) > + if (cmd->use_sg) { > + sbp2_command_orb_map_scatterlist(orb); > + } else if (cmd->request_bufflen > SBP2_MAX_SG_ELEMENT_LENGTH) { > + /* FIXME: Need to split this into a sg list... but > + * could we get the scsi or blk layer to do that by > + * reporting our max supported block size? */ > + fw_error("command > 64k\n"); > + goto fail_bufflen; > + } else if (cmd->request_bufflen > 0) { > + sbp2_command_orb_map_buffer(orb); > + } The use_sg == 0, request_bufflen != 0 case can't happen anymore. > + fail_mapping: > + kfree(orb); > + fail_alloc: > + cmd->result = DID_ERROR << 16; > + done(cmd); Failure due to ressource shortage should not complete the command but return SCSI_MLQUEUE_HOST_BUSY/SCSI_MLQUEUE_DEVICE_BUSY. > + return 0; > +} > +static struct scsi_host_template scsi_driver_template = { > + .module = THIS_MODULE, > + .name = "SBP-2 IEEE-1394", > + .proc_name = (char *)sbp2_driver_name, Please don't use casrs here. Either fix up the definition so it accepts const strings or pass a non-const one. > +static int add_scsi_devices(struct fw_unit *unit) > +{ > + struct sbp2_device *sd = unit->device.driver_data; > + int retval, lun; > + > + if (sd->scsi_host != NULL) > + return 0; > + > + sd->scsi_host = scsi_host_alloc(&scsi_driver_template, > + sizeof(unsigned long)); > + if (sd->scsi_host == NULL) { > + fw_error("failed to register scsi host\n"); > + return -1; > + } > + > + sd->scsi_host->hostdata[0] = (unsigned long)unit; Please take a look ar ther other scsi drivers how this is supposed to be used. > + retval = scsi_add_host(sd->scsi_host, &unit->device); > + if (retval < 0) { > + fw_error("failed to add scsi host\n"); > + scsi_host_put(sd->scsi_host); > + sd->scsi_host = NULL; > + return retval; > + } > + > + /* FIXME: Loop over luns here. */ > + lun = 0; > + retval = scsi_add_device(sd->scsi_host, 0, 0, lun); > + if (retval < 0) { > + fw_error("failed to add scsi device\n"); > + scsi_remove_host(sd->scsi_host); > + scsi_host_put(sd->scsi_host); > + sd->scsi_host = NULL; > + return retval; > + } > + > + return 0; > +} Do we really need another scanning algorithm? Can't you use scsi_scan_target instead and let the core scsi code handle the scanning? > + > +static void remove_scsi_devices(struct fw_unit *unit) > +{ > + struct sbp2_device *sd = unit->device.driver_data; > + > + if (sd->scsi_host != NULL) { > + scsi_remove_host(sd->scsi_host); > + scsi_host_put(sd->scsi_host); > + } > + sd->scsi_host = NULL; > +} This function seems rather oddly named. And the checking and setting of scsi_host looks like you have some lifetime rule problems. - 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