Re: [PATCH 2/6] hpsa: add support for legacy boards
On Wed, Aug 09, 2017 at 05:08:05PM +0200, Hannes Reinecke wrote: > On 08/09/2017 03:41 PM, Christoph Hellwig wrote: > > On Tue, Aug 08, 2017 at 10:35:11AM +0200, Hannes Reinecke wrote: > >> Add support for legacy boards, ensuring to enable the driver for > >> those boards only when 'hpsa_allow_any' is set. > > > > Why the wildcard instead of the specific IDs from the cciss driver? > > > Because I'm lazy? And we're catching those boards with the wildcard > entry anyway? And the distinction between wildcard entry (designed to > catch unsupported devices) and pci IDs from the cciss drivers (which > will be unsupported, too) is meaningless? So what again is unsupported? How do we know there never was any other compaq device claiming to be raid class?
Re: [PATCH 2/6] hpsa: add support for legacy boards
On 08/09/2017 03:41 PM, Christoph Hellwig wrote: > On Tue, Aug 08, 2017 at 10:35:11AM +0200, Hannes Reinecke wrote: >> Add support for legacy boards, ensuring to enable the driver for >> those boards only when 'hpsa_allow_any' is set. > > Why the wildcard instead of the specific IDs from the cciss driver? > Because I'm lazy? And we're catching those boards with the wildcard entry anyway? And the distinction between wildcard entry (designed to catch unsupported devices) and pci IDs from the cciss drivers (which will be unsupported, too) is meaningless? But if you insist ... Cheers, Hannes -- Dr. Hannes ReineckeTeamlead Storage & Networking h...@suse.de +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)
Re: [PATCH 2/6] hpsa: add support for legacy boards
On 08/09/2017 03:45 PM, Christoph Hellwig wrote:
>> -static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id)
>> +static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id,
>> +bool *supported)
>> {
>> int i;
>> u32 subsystem_vendor_id, subsystem_device_id;
>> @@ -7242,9 +7266,22 @@ static int hpsa_lookup_board_id(struct pci_dev *pdev,
>> u32 *board_id)
>> *board_id = ((subsystem_device_id << 16) & 0x) |
>> subsystem_vendor_id;
>>
>> +if (supported)
>> +*supported = true;
>> for (i = 0; i < ARRAY_SIZE(products); i++)
>> -if (*board_id == products[i].board_id)
>> -return i;
>> +if (*board_id == products[i].board_id) {
>> +if (products[i].access != _access &&
>> +products[i].access != _access)
>> +return i;
>> +if (hpsa_allow_any) {
>> +dev_warn(>dev,
>> + "unsupported board ID: 0x%08x\n",
>> + *board_id);
>> +if (supported)
>> +*supported = false;
>> +return i;
>> +}
>> +}
>
> Can you explain the point of the supported flag?
>
'hpsa_allow_any' just enables the _driver_ to bind to older /
unsupported boards, it doesn't tell us if this particular instance is an
old board.
For older boards we should suppress messages from not-implemented
features, whereas on 'real' boards those features should be implemented,
and we should be throwing an error or a warning.
Hence the =supported" flag.
>> +unsigned long register_value =
>> +readl(h->vaddr + SA5_INTR_STATUS);
>> +return (register_value & SA5B_INTR_PENDING);
>
> This should be condensed into:
>
> return readl(h->vaddr + SA5_INTR_STATUS) & SA5B_INTR_PENDING;
>
Yeah; but this is _actually_ just copied over, so other lines will be
affected here, too.
Will be adding a patch for that.
>> .command_completed = SA5_completed,
>> };
>>
>> +/* Duplicate entry of the above to mark unsupported boards */
>> +static struct access_method SA5A_access = {
>> +.submit_command = SA5_submit_command,
>> +.set_intr_mask = SA5_intr_mask,
>> +.intr_pending = SA5_intr_pending,
>> +.command_completed = SA5_completed,
>> +};
>> +
>> +static struct access_method SA5B_access = {
>> +.submit_command = SA5_submit_command,
>> +.set_intr_mask = SA5B_intr_mask,
>> +.intr_pending = SA5B_intr_pending,
>> +.command_completed = SA5_completed,
>> +};
>
> Please align the fields nicely, e.g.:
>
> static struct access_method SA5A_access = {
> .submit_command = SA5_submit_command,
> .set_intr_mask = SA5_intr_mask,
> ...
>
Okay.
Cheers,
Hannes
--
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de +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)
Re: [PATCH 2/6] hpsa: add support for legacy boards
> -static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id)
> +static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id,
> + bool *supported)
> {
> int i;
> u32 subsystem_vendor_id, subsystem_device_id;
> @@ -7242,9 +7266,22 @@ static int hpsa_lookup_board_id(struct pci_dev *pdev,
> u32 *board_id)
> *board_id = ((subsystem_device_id << 16) & 0x) |
> subsystem_vendor_id;
>
> + if (supported)
> + *supported = true;
> for (i = 0; i < ARRAY_SIZE(products); i++)
> - if (*board_id == products[i].board_id)
> - return i;
> + if (*board_id == products[i].board_id) {
> + if (products[i].access != _access &&
> + products[i].access != _access)
> + return i;
> + if (hpsa_allow_any) {
> + dev_warn(>dev,
> + "unsupported board ID: 0x%08x\n",
> + *board_id);
> + if (supported)
> + *supported = false;
> + return i;
> + }
> + }
Can you explain the point of the supported flag?
> + unsigned long register_value =
> + readl(h->vaddr + SA5_INTR_STATUS);
> + return (register_value & SA5B_INTR_PENDING);
This should be condensed into:
return readl(h->vaddr + SA5_INTR_STATUS) & SA5B_INTR_PENDING;
> .command_completed = SA5_completed,
> };
>
> +/* Duplicate entry of the above to mark unsupported boards */
> +static struct access_method SA5A_access = {
> + .submit_command = SA5_submit_command,
> + .set_intr_mask = SA5_intr_mask,
> + .intr_pending = SA5_intr_pending,
> + .command_completed = SA5_completed,
> +};
> +
> +static struct access_method SA5B_access = {
> + .submit_command = SA5_submit_command,
> + .set_intr_mask = SA5B_intr_mask,
> + .intr_pending = SA5B_intr_pending,
> + .command_completed = SA5_completed,
> +};
Please align the fields nicely, e.g.:
static struct access_method SA5A_access = {
.submit_command = SA5_submit_command,
.set_intr_mask = SA5_intr_mask,
...
Re: [PATCH 2/6] hpsa: add support for legacy boards
On Tue, Aug 08, 2017 at 10:35:11AM +0200, Hannes Reinecke wrote: > Add support for legacy boards, ensuring to enable the driver for > those boards only when 'hpsa_allow_any' is set. Why the wildcard instead of the specific IDs from the cciss driver?
