Hi Christoph,
Thanks for the review!
I know it's a pain
On 03/15/2018 09:37 AM, Christoph Hellwig wrote:
>> +#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.
>
I guess we can just drop this check.
> And to avoid some boiler plate code it could just return
> cmd_blk->status as the return value.
>
Ok.
>> +
>> +/*
>> + 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.
>
Copied over from the original driver.
Will be changing it.
>> + 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.
>
True.
>> + 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?
>
I tried to do some fancy computation (only display a change if the
device actually _is_ broken), but then we do want to display a change
from a broken/failed device to a working one, too.
So yeah, it should be &&s.
>> + if ((new->pdev_dead > 0 ||
>> + new->pdev_dead != old.pdev_dead) ||
>
> Same here.
>
No, here the '||' is actually okay, as we only want to update the bgi
status for dead devices.
(I think ...)
>> +static sssize_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.
>
Oh, it surely isn't.
But the BIOS is _soo_ bitchy that the last thing I'd try it to put it
into a non-x86 machine.
So I'd rather ignore it.
>> +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.
>
Okay.
>> +/*
>> + 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.
>
Will be fixed together with the docbook updates.
>
> Note that most of these comments apply very similarly to the myrs driver
> as well.
>
Yes, will be checking.
Thanks for reviewing.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@xxxxxxx +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
