Messages in this thread | | | Subject | Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid | From | Tom Rix <> | Date | Thu, 27 Jan 2022 16:32:37 -0800 |
| |
On 1/27/22 2:47 PM, Nick Desaulniers wrote: > + Miguel (the clang-format maintainer), Joe (checkpatch maintainer) > These criticisms are worth reviewing. > > On Thu, Jan 27, 2022 at 2:38 PM Finn Thain <fthain@linux-m68k.org> wrote: >> >> On Thu, 27 Jan 2022, trix@redhat.com wrote: >> >>> From: Tom Rix <trix@redhat.com> >>> >>> checkpatch reports several hundred formatting errors. Run these files >>> through clang-format and knock off some of them. >>> >> That method seems like a good recipe for endless churn unless checkpatch >> and clang-format really agree about these style rules. >> >> Why use checkpatch to assess code style, if we could simply diff the >> existing source with the output from clang-format... but it seems that >> clang-format harms readability, makes indentation errors and uses >> inconsistent style rules. Some examples:
Problems with clang-format should be fixed, I'll take a look.
I was reviewing this file for another issue and could not get past how horredously bad it was and really did not want to manually fix the 400+ formatting errors. I will drop this patch and use the use these files to verify the .clang-format .
Tom
>> >>> static unsigned short int max_sectors_per_io = MAX_SECTORS_PER_IO; >>> module_param(max_sectors_per_io, ushort, 0); >>> -MODULE_PARM_DESC(max_sectors_per_io, "Maximum number of sectors per I/O request (default=MAX_SECTORS_PER_IO=128)"); >>> - >>> +MODULE_PARM_DESC( >>> + max_sectors_per_io, >>> + "Maximum number of sectors per I/O request (default=MAX_SECTORS_PER_IO=128)"); >>> >>> static unsigned short int max_mbox_busy_wait = MBOX_BUSY_WAIT; >>> module_param(max_mbox_busy_wait, ushort, 0); >>> -MODULE_PARM_DESC(max_mbox_busy_wait, "Maximum wait for mailbox in microseconds if busy (default=MBOX_BUSY_WAIT=10)"); >>> +MODULE_PARM_DESC( >>> + max_mbox_busy_wait, >>> + "Maximum wait for mailbox in microseconds if busy (default=MBOX_BUSY_WAIT=10)"); >>> >> This code is longer for no real improvement. >> >>> /* >>> * The File Operations structure for the serial/ioctl interface of the driver >>> */ >>> static const struct file_operations megadev_fops = { >>> - .owner = THIS_MODULE, >>> - .unlocked_ioctl = megadev_unlocked_ioctl, >>> - .open = megadev_open, >>> - .llseek = noop_llseek, >>> + .owner = THIS_MODULE, >>> + .unlocked_ioctl = megadev_unlocked_ioctl, >>> + .open = megadev_open, >>> + .llseek = noop_llseek, >>> }; >>> >>> /* >> Readability loss. >> >>> - prod_info_dma_handle = dma_map_single(&adapter->dev->dev, >>> - (void *)&adapter->product_info, >>> - sizeof(mega_product_info), >>> - DMA_FROM_DEVICE); >>> + prod_info_dma_handle = dma_map_single( >>> + &adapter->dev->dev, (void *)&adapter->product_info, >>> + sizeof(mega_product_info), DMA_FROM_DEVICE); >>> >> Note the orphaned first parameter and odd indentation. >> >>> static DEF_SCSI_QCMD(megaraid_queue) >>> >>> -/** >>> + /** >>> * mega_allocate_scb() >>> * @adapter: pointer to our soft state >>> * @cmd: scsi command from the mid-layer >> Indentation error. >> >>> @@ -418,15 +409,14 @@ static DEF_SCSI_QCMD(megaraid_queue) >>> * Allocate a SCB structure. This is the central structure for controller >>> * commands. >>> */ >>> -static inline scb_t * >>> -mega_allocate_scb(adapter_t *adapter, struct scsi_cmnd *cmd) >>> + static inline scb_t *mega_allocate_scb(adapter_t *adapter, >>> + struct scsi_cmnd *cmd) >>> { >>> struct list_head *head = &adapter->free_list; >> Same. >> >>> @@ -586,26 +568,25 @@ mega_build_cmd(adapter_t *adapter, struct scsi_cmnd *cmd, int *busy) >>> >>> ldrv_num = mega_get_ldrv_num(adapter, cmd, channel); >>> >>> - >>> max_ldrv_num = (adapter->flag & BOARD_40LD) ? >>> - MAX_LOGICAL_DRIVES_40LD : MAX_LOGICAL_DRIVES_8LD; >>> + MAX_LOGICAL_DRIVES_40LD : >>> + MAX_LOGICAL_DRIVES_8LD; >>> >> Churn, if not readability loss. Note the indentation change here is >> inconsistent with the indentation change noted above. >> >>> * 6-byte READ(0x08) or WRITE(0x0A) cdb >>> */ >>> - if( cmd->cmd_len == 6 ) { >>> - mbox->m_out.numsectors = (u32) cmd->cmnd[4]; >>> - mbox->m_out.lba = >>> - ((u32)cmd->cmnd[1] << 16) | >>> - ((u32)cmd->cmnd[2] << 8) | >>> - (u32)cmd->cmnd[3]; >>> + if (cmd->cmd_len == 6) { >>> + mbox->m_out.numsectors = (u32)cmd->cmnd[4]; >>> + mbox->m_out.lba = ((u32)cmd->cmnd[1] << 16) | >>> + ((u32)cmd->cmnd[2] << 8) | >>> + (u32)cmd->cmnd[3]; >>> >>> mbox->m_out.lba &= 0x1FFFFF; >>> >> Here, the orphaned term is moved up, next to the =. And yet, >> >>> /* Calculate Scatter-Gather info */ >>> - mbox->m_out.numsgelements = mega_build_sglist(adapter, scb, >>> - (u32 *)&mbox->m_out.xferaddr, &seg); >>> + mbox->m_out.numsgelements = >>> + mega_build_sglist(adapter, scb, >>> + (u32 *)&mbox->m_out.xferaddr, >>> + &seg); >>> >>> return scb; >>> >> ... here the first term is moved down and orphaned, which is another >> inconsistency. > >
| |