> +#define myrb_logical_channel(shost) ((shost)->max_channel - 1)
inline function, please.
> +/*
> + * myrb_exec_cmd executes V1 Command and waits for completion.
> + */
> +
> +static void myrb_exec_cmd(myrb_hba *cb, myrb_cmdblk *cmd_blk)
> +{
> + DECLARE_COMPLETION_ONSTACK(Completion);
> + unsigned long flags;
> +
> + cmd_blk->Completion = &Completion;
> +
> + spin_lock_irqsave(&cb->queue_lock, flags);
> + cb->qcmd(cb, cmd_blk);
> + spin_unlock_irqrestore(&cb->queue_lock, flags);
> +
> + if (in_interrupt())
> + return;
> + wait_for_completion(&Completion);
> +}
This interface looks completely bogus as it silently does something
else if in_interrupt() is true. As far as I can tell from a quick
scan it never even is called from interrupt context and all callers
expect to get a status back, so it should be changed to just
a WARN_ON for the in_interrupt case.
And to avoid some boiler plate code it could just return
cmd_blk->status as the return value.
> +
> +/*
> + myrb_exec_type3 executes a DAC960 V1 Firmware Controller Type 3
> + Command and waits for completion.
> +*/
> +
> +static unsigned short myrb_exec_type3(myrb_hba *cb,
> + myrb_cmd_opcode op,
> + dma_addr_t addr)
Why the empty lines before the description and function? Also
Please use normal Linux block comment style instead of this weird
style.
> + dma_free_coherent(&cb->pdev->dev, sizeof(myrb_log_entry),
> + ev_buf, ev_addr);
> + return;
> +}
No need for an empty return statement at the end of a function.
> + if ((new_entry->parity_err != old_entry->parity_err) ||
> + (new_entry->soft_err != old_entry->soft_err) ||
> + (new_entry->hard_err != old_entry->hard_err) ||
> + (new_entry->misc_err !=
> + old_entry->misc_err))
No need for any of the inner braces.
> + mbox->Type3.addr = rbld_addr;
> + myrb_exec_cmd(cb, cmd_blk);
> + status = cmd_blk->status;
> + if (status == DAC960_V1_NormalCompletion) {
> + unsigned int ldev_num = rbld_buf->ldev_num;
> + unsigned int ldev_size = rbld_buf->ldev_size;
> + unsigned int blocks_done =
> + ldev_size - rbld_buf->blocks_left;
> + struct scsi_device *sdev;
> +
> + sdev = scsi_device_lookup(cb->host,
> + myrb_logical_channel(cb->host),
> + ldev_num, 0);
This seems to leak the scsi_device.
> + if ((new->ldev_critical > 0 ||
> + new->ldev_critical != old.ldev_critical) ||
> + (new->ldev_offline > 0 ||
> + new->ldev_offline != old.ldev_offline) ||
> + (new->ldev_count != old.ldev_count)) {
no need for the inner braces here as-is, but the logic looks broken to
me as well. Shouldn't the inner ||s be &&s?
> + if ((new->pdev_dead > 0 ||
> + new->pdev_dead != old.pdev_dead) ||
Same here.
> +static ssize_t myrb_show_dev_level(struct device *dev,
> + struct device_attribute *attr, char *buf)
Two tab indentation for prototype continuations, please.
> +myrb_hba *myrb_alloc_host(struct pci_dev *pdev,
> + const struct pci_device_id *entry)
static? Or even bettetr just merge into the caller.
> +{
> + struct Scsi_Host *shost;
> + myrb_hba *cb;
> +
> + shost = scsi_host_alloc(&myrb_template, sizeof(myrb_hba));
> + if (!shost)
> + return NULL;
> +
> + cb = (myrb_hba *)shost->hostdata;
Use shost_priv(), please.
> +bool myrb_err_status(myrb_hba *cb, unsigned char error,
> + unsigned char parm0, unsigned char parm1)
static for all functions, please.
> +static void myrb_remove(struct pci_dev *pdev)
> +{
> + myrb_hba *cb = pci_get_drvdata(pdev);
> +
> + if (cb == NULL)
> + return;
Can't happen.
> +static const struct pci_device_id myrb_id_table[] = {
> + {
> + .vendor = PCI_VENDOR_ID_DEC,
> + .device = PCI_DEVICE_ID_DEC_21285,
> + .subvendor = PCI_VENDOR_ID_MYLEX,
> + .subdevice = PCI_DEVICE_ID_MYLEX_DAC960_LA,
> + .driver_data = (unsigned long) &DAC960_LA_privdata,
> + },
> + {
> + .vendor = PCI_VENDOR_ID_MYLEX,
> + .device = PCI_DEVICE_ID_MYLEX_DAC960_PG,
> + .subvendor = PCI_ANY_ID,
> + .subdevice = PCI_ANY_ID,
> + .driver_data = (unsigned long) &DAC960_PG_privdata,
> + },
Please use the PCI_DEVICE_SUB and PCI_VDEVICE macros.
> +typedef enum
No typedefs for enums or structs, please.
> +{
Linux style is to not have the opening curly brace on a separate line.
> +}
> +__attribute__ ((packed))
The attribute should be on the line above.
> +
> +/*
> + Define the DAC960 V1 Firmware Get Logical Drive Information Command
> + reply structure.
> +*/
> +
> +typedef myrb_ldev_info myrb_ldev_info_arr[MYRB_MAX_LDEVS];
No need for this typedef. Use a pointer in the containing structure,
and multiply the size by MYRB_MAX_LDEVS for the sizeofs.
> +typedef struct myrb_sge_s
And once you convert these to structs please drop the _s prefixes.
> +{
> + u32 sge_addr; /* Bytes 0-3 */
> + u32 sge_count; /* Bytes 4-7 */
None of this looks endian clean. But I guess that would be too much
to expect from such a legacy driver conversion.
> +static inline
> +void DAC960_LA_HardwareMailboxNewCommand(void __iomem *base)
> +{
> + DAC960_LA_InboundDoorBellRegister_T InboundDoorBellRegister;
> + InboundDoorBellRegister.All = 0;
> + InboundDoorBellRegister.Write.HardwareMailboxNewCommand = true;
> + writeb(InboundDoorBellRegister.All,
> + base + DAC960_LA_InboundDoorBellRegisterOffset);
> +}
Please move all these helpers to the .c file, especially as they are used
for function pointers and the inline makes no sense at all. And give the
symbols readable names. Also using strange union types for bytes just makes
a mess by not being readabable and violating union aliasing rules.
The above could be a simple:
static inline void dac960_la_hwmbx_newcmd(void __iomem *base)
{
writeb(DAC960_HWMBX_NEW_CMD, base + DAC960_LA_INBOUND_DB_OFF);
}
With the same crap applied to all the helpers below.
> +/*
> + Define the structure of the DAC960 PD Series Outbound Door Bell Register.
> +*/
"Define the structure of the" is redundant here and in many other comments.
Note that most of these comments apply very similarly to the myrs driver
as well.
