Re: Subject: [PATCH 1/3] Let scsi_cmnd-cmnd use request-cmd buffer
On Tue, Feb 12 2008 at 21:41 +0200, James Bottomley [EMAIL PROTECTED] wrote: On Sun, 2008-02-10 at 21:05 +0200, Boaz Harrosh wrote: - struct scsi_cmnd had a 16 bytes command buffer of its own. This is an unnecessary duplication and copy of request's cmd. It is probably left overs from the time that scsi_cmnd could function without a request attached. So clean that up. - Once above is done, few places, apart from scsi-ml, needed adjustments due to changing the data type of scsi_cmnd-cmnd. - Lots of drivers still use MAX_COMMAND_SIZE. So I have left that #define but equate it to BLK_MAX_CDB. The way I see it and is reflected in the patch below is. MAX_COMMAND_SIZE - means: The longest fixed-length (*) SCSI CDB as per the SCSI standard and is not related to the implementation. BLK_MAX_CDB. - The allocated space at the request level (*)fixed-length here means commands that their size can be determined by their opcode and the CDB does not carry a length specifier, like the VARIABLE_LENGTH_CMD(0x7f) command. This is actually not exactly true and the SCSI standard also defines extended commands and vendor specific commands that can be bigger than 16 bytes. The kernel will support these using the same infrastructure used for VARLEN CDB's. So in effect MAX_COMMAND_SIZE means the maximum size command scsi-ml supports without specifying a cmd_len by ULD's When we do this, what happens to the minority of drivers that need the command in DMAable memory ... or have you audited them all and we can now dump the DMA pool allocation for SCSI commands? James Am I right in assuming that I only need to audited the drivers that have .unchecked_isa_dma set? I will redo this audit again, and report back. Boaz - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Subject: [PATCH 1/3] Let scsi_cmnd-cmnd use request-cmd buffer
On Tue, Feb 12 2008 at 19:45 +0200, Christoph Hellwig [EMAIL PROTECTED] wrote: On Sun, Feb 10, 2008 at 09:05:17PM +0200, Boaz Harrosh wrote: - Lots of drivers still use MAX_COMMAND_SIZE. So I have left that #define but equate it to BLK_MAX_CDB. The way I see it and is reflected in the patch below is. MAX_COMMAND_SIZE - means: The longest fixed-length (*) SCSI CDB as per the SCSI standard and is not related to the implementation. BLK_MAX_CDB. - The allocated space at the request level (*)fixed-length here means commands that their size can be determined by their opcode and the CDB does not carry a length specifier, like the VARIABLE_LENGTH_CMD(0x7f) command. This is actually not exactly true and the SCSI standard also defines extended commands and vendor specific commands that can be bigger than 16 bytes. The kernel will support these using the same infrastructure used for VARLEN CDB's. So in effect MAX_COMMAND_SIZE means the maximum size command scsi-ml supports without specifying a cmd_len by ULD's A comment like this should be near the declaration of MAX_COMMAND_SIZE +#define MAX_COMMAND_SIZE 16 +#if (MAX_COMMAND_SIZE BLK_MAX_CDB) +# error MAX_COMMAND_SIZE can not be smaller than BLK_MAX_CDB +#endif No tabs between the # and the rest of the cpp command, please. Either nothing or a single space as indentation instead. Except for those two small nitpicks this looks very good to me. Nice memory saving aswel. Agree with both comments. Thanks for the review, will fix in the next submission. Boaz - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Subject: [PATCH 1/3] Let scsi_cmnd-cmnd use request-cmd buffer
On Sun, 2008-02-10 at 21:05 +0200, Boaz Harrosh wrote: - struct scsi_cmnd had a 16 bytes command buffer of its own. This is an unnecessary duplication and copy of request's cmd. It is probably left overs from the time that scsi_cmnd could function without a request attached. So clean that up. - Once above is done, few places, apart from scsi-ml, needed adjustments due to changing the data type of scsi_cmnd-cmnd. - Lots of drivers still use MAX_COMMAND_SIZE. So I have left that #define but equate it to BLK_MAX_CDB. The way I see it and is reflected in the patch below is. MAX_COMMAND_SIZE - means: The longest fixed-length (*) SCSI CDB as per the SCSI standard and is not related to the implementation. BLK_MAX_CDB. - The allocated space at the request level (*)fixed-length here means commands that their size can be determined by their opcode and the CDB does not carry a length specifier, like the VARIABLE_LENGTH_CMD(0x7f) command. This is actually not exactly true and the SCSI standard also defines extended commands and vendor specific commands that can be bigger than 16 bytes. The kernel will support these using the same infrastructure used for VARLEN CDB's. So in effect MAX_COMMAND_SIZE means the maximum size command scsi-ml supports without specifying a cmd_len by ULD's When we do this, what happens to the minority of drivers that need the command in DMAable memory ... or have you audited them all and we can now dump the DMA pool allocation for SCSI commands? James - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Subject: [PATCH 1/3] Let scsi_cmnd-cmnd use request-cmd buffer
On Sun, Feb 10, 2008 at 09:05:17PM +0200, Boaz Harrosh wrote: - Lots of drivers still use MAX_COMMAND_SIZE. So I have left that #define but equate it to BLK_MAX_CDB. The way I see it and is reflected in the patch below is. MAX_COMMAND_SIZE - means: The longest fixed-length (*) SCSI CDB as per the SCSI standard and is not related to the implementation. BLK_MAX_CDB. - The allocated space at the request level (*)fixed-length here means commands that their size can be determined by their opcode and the CDB does not carry a length specifier, like the VARIABLE_LENGTH_CMD(0x7f) command. This is actually not exactly true and the SCSI standard also defines extended commands and vendor specific commands that can be bigger than 16 bytes. The kernel will support these using the same infrastructure used for VARLEN CDB's. So in effect MAX_COMMAND_SIZE means the maximum size command scsi-ml supports without specifying a cmd_len by ULD's A comment like this should be near the declaration of MAX_COMMAND_SIZE +#define MAX_COMMAND_SIZE 16 +#if (MAX_COMMAND_SIZE BLK_MAX_CDB) +#error MAX_COMMAND_SIZE can not be smaller than BLK_MAX_CDB +#endif No tabs between the # and the rest of the cpp command, please. Either nothing or a single space as indentation instead. Except for those two small nitpicks this looks very good to me. Nice memory saving aswel. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
