* RE: AACRAID driver update patch version - 26400
2010-05-01 19:03 ` James Bottomley
@ 2010-05-10 11:05 ` Rajashekhara, Mahesh
2010-05-10 11:12 ` [PATCH 2/5] AACRAID driver update Rajashekhara, Mahesh
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Rajashekhara, Mahesh @ 2010-05-10 11:05 UTC (permalink / raw)
To: James Bottomley, linux-scsi@vger.kernel.org; +Cc: AACRAID
Hi James,
Thanks for your comments.
We have prepared a set of driver patches(one per change) against latest scsi-misc-2.6 kernel.
The total number of changes between the driver what we have today (version: 26400) and the last submitted driver(24702) is 5.
I am sending separate e-mail for each patch.
Below is one of the driver patch details:
Problem description:
--------------------
When the JBOD is created from the OS using Adaptec Storage Manager utility device is not available under FDISK until a system restart is done.
Solution:
---------
AIF handling: If there is a JBOD drive added to the system, identify the old one with scsi_device_lookup() and remove it to enable a fresh scsi_add_device(); else the new JBOD is not available until reboot.
Please see patch below.
=====================
Mahesh
Linux Driver Development Engineer,
Adaptec ODC, Bangalore
This attached patch is against current scsi-misc-2.6.
ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments (inline gets damaged, please use attachment).
Signed-off-by: Mahesh Rajashekhara <aacraid@adaptec.com>
drivers/scsi/aacraid/aacraid.h | 2 +-
drivers/scsi/aacraid/commsup.c | 10 ++++++++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
--- a/drivers/scsi/aacraid/aacraid.h 2010-05-13 10:12:33.041953680 -0700
+++ b/drivers/scsi/aacraid/aacraid.h 2010-05-13 10:16:33.613381264 -0700
@@ -12,7 +12,7 @@
*----------------------------------------------------------------------------*/
#ifndef AAC_DRIVER_BUILD
-# define AAC_DRIVER_BUILD 24702
+# define AAC_DRIVER_BUILD 26000
# define AAC_DRIVER_BRANCH "-ms"
#endif
#define MAXIMUM_NUM_CONTAINERS 32
diff -ru a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
--- a/drivers/scsi/aacraid/commsup.c 2010-05-13 10:12:33.042953528 -0700
+++ b/drivers/scsi/aacraid/commsup.c 2010-05-13 10:52:35.768683696 -0700
@@ -966,6 +966,16 @@
device_config_needed =
(((__le32 *)aifcmd->data)[0] ==
cpu_to_le32(AifEnAddJBOD)) ? ADD : DELETE;
+ if (device_config_needed == ADD) {
+ device = scsi_device_lookup(dev->scsi_host_ptr,
+ channel,
+ id,
+ lun);
+ if (device) {
+ scsi_remove_device(device);
+ scsi_device_put(device);
+ }
+ }
break;
case AifEnEnclosureManagement:
Regards - Mahesh Rajashekhara
-----Original Message-----
From: James Bottomley [mailto:James.Bottomley@suse.de]
Sent: Sunday, May 02, 2010 12:33 AM
To: Rajashekhara, Mahesh
Cc: linux-scsi@vger.kernel.org; AACRAID
Subject: Re: AACRAID driver update patch version - 26400
On Wed, 2010-04-07 at 20:55 -0700, Rajashekhara, Mahesh wrote:
> (Resending in plain text as the earlier email got bounced)
>
> Hi All,
>
> We have prepared driver patch from latest driver source.
>
> We worked on a issue related to SMARTCTL([Bugme-new] [Bug 15623] New: doesn't allow SMART-passthrough on Adaptec Series 5 controller)
> and found the root cause of this problem. We have responded to this bug, stating that we will be releasing the driver patch for addressing this problem.
> Also, in our recent program releases we have addressed some more issues.
>
> Please look into the release notes.
Please, no. What I actually need is a set of patches (one per change or
feature) with a properly added description and signoff as described in
Documentation/SubmittingPatches.
> Please find the attachments.
> 1. aacraid-version-26400-040510-patch - Driver patch
> 2. aacraid-scsi-misc-2.6-warnings.txt - Warning message text
> 3. Release notes
So if I look at the Changelog it seems that there should be four
separate changes:
> AACRAID Driver for Linux - version 26400
> ----------------------------------------
>
> Addressed issues:
> ----------------
> 1. The default driver setting is "expose_physicals=0", which means raw
> physical drives not exposed to OS.
> If the user wants to expose connected physical drives, enable
> "expose_physicals" module parameter.
> With the new JBOD firmware, physical drives are not available for
> "expose_physicals>0".
> In function "aac_expose_phy_device", added to reset the appropriate
> bit in the first byte of inquiry data.
> This fix exposes the connected physical drives.
OK
> 2. Added support for handling ATA pass-through commands.
> When the "CC" bit is set by the host in ATA pass-through CDB,
> driver is supposed to return DID_OK
> When the "CC" bit is reset by the host, driver should return
> DID_ERROR
No, that's not correct. You should return a result code of
SAM_STAT_CHECK_CONDITION, with a driver byte of DID_OK. If you return
DID_ERROR, it will unconditionally trigger error handling and a retry.
> 3. Firmware exposes lesser container capacity than the actual one.
> It exposes [actaul size - hidden reserved space(10MB)] to the OS,
> IO's to the 10MB should be prohibhited from the Linux
> driver.
> When the IO's falls into hidden reserved space, driver internally
> sets sensekey to HARDWARE_ERROR and sends notification to the SCSI mid
> layer.
> Added "expose_hidden_space" flag, by default the fix will be
> executed.
> Only if the user sets "expose_hidden_space=1", user can access
> beyond the array reported size(hidden reserved space 10MB).
This doesn't exactly look like the right interface. Won't the user want
the hidden space exposed on a per array basis? In which case a sysfs
flag in the host attributes would be far more appropriate than a module
flag?
> 4. Based on the notification from firmware, the driver sets up
> SDV_OFFLINE flag for a deleted array and
> the SCSI mid layer comes to know the attached SCSI device has gone
> offline. But with
> this notification, scsi device entries are not being removed.
> Enabled "AAC_DEBUG_INSTRUMENT_AIF_DELETE" flag in the header file,
> with this flag the driver uses "scsi_remove_device".
> On the notification from firmware, the driver calls
> "scsi_remove_device" for the deleted array.
> This call not only informs the scsi device status to the SCSI mid
> layer but also
> removes corresponding scsi device entry from the linux
> sysfs.
OK, don't understand this. As noted below, the define
AAC_DEBUG_INSTRUMENT_AIF_DELETE is set by your patch, but never appears
at all in the driver ... is there missing code?
> Thanks & Regards,
> Mahesh
> Linux Driver Development Engineer,
> Adaptec ODC, Bangalore
> diff -ru a/drivers/scsi/aacraid/aachba.c
> b/drivers/scsi/aacraid/aachba.c
> --- a/drivers/scsi/aacraid/aachba.c 2010-04-03 05:49:52.000000000
> -0700
> +++ b/drivers/scsi/aacraid/aachba.c 2010-04-07 12:35:36.468017040
> -0700
> @@ -109,6 +109,16 @@
> #define BYTE2(x) (unsigned char)((x) >> 16)
> #define BYTE3(x) (unsigned char)((x) >> 24)
>
> +
> +/* ATA pass thru commands */
> +#ifndef ATA_12
> +#define ATA_12 0xa1 /* 12-byte pass-thru */
> +#endif
> +
> +#ifndef ATA_16
> +#define ATA_16 0x85 /* 16-byte pass-thru */
> +#endif
> +
This chunk is clearly wrong ... those defines are in include/scsi/scsi.h
> @@ -1647,6 +1671,27 @@
> count = (scsicmd->cmnd[7] << 8) | scsicmd->cmnd[8];
> break;
> }
> +
> + if (expose_hidden_space <= 0) {
> + if ((lba + count) > (dev->fsa_dev[scmd_id(scsicmd)].size)) {
> + int cid = scmd_id(scsicmd);
> + dprintk((KERN_DEBUG "aacraid: Illegal lba\n"));
> + scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
> + SAM_STAT_CHECK_CONDITION;
> + set_sense(&dev->fsa_dev[cid].sense_data,
> + HARDWARE_ERROR,
> + SENCODE_INTERNAL_TARGET_FAILURE,
> + ASENCODE_INTERNAL_TARGET_FAILURE, 0, 0);
> + memcpy(scsicmd->sense_buffer,
> + &dev->fsa_dev[cid].sense_data,
> + min_t(size_t,
> + sizeof(dev->fsa_dev[cid].sense_data),
> + SCSI_SENSE_BUFFERSIZE));
> + scsicmd->scsi_done(scsicmd);
> + return 1;
> + }
> + }
> +
This chunk is correctly indented, that's great.
> dprintk((KERN_DEBUG "aac_read[cpu %d]: lba = %llu, t = %ld.\n",
> smp_processor_id(), (unsigned long long)lba, jiffies));
> if (aac_adapter_bounds(dev,scsicmd,lba))
> @@ -1727,6 +1772,26 @@
> count = (scsicmd->cmnd[7] << 8) | scsicmd->cmnd[8];
> fua = scsicmd->cmnd[1] & 0x8;
> }
> +
> + if (expose_hidden_space <= 0) {
> + if ((lba + count) > (dev->fsa_dev[scmd_id(scsicmd)].size)) {
> + int cid = scmd_id(scsicmd);
> + dprintk((KERN_DEBUG "aacraid: Illegal lba\n"));
> + scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
> + SAM_STAT_CHECK_CONDITION;
> + set_sense(&dev->fsa_dev[cid].sense_data,
> + HARDWARE_ERROR, SENCODE_INTERNAL_TARGET_FAILURE,
> + ASENCODE_INTERNAL_TARGET_FAILURE, 0, 0);
> + memcpy(scsicmd->sense_buffer,
> + &dev->fsa_dev[cid].sense_data,
> + min_t(size_t,
> + sizeof(dev->fsa_dev[cid].sense_data),
> + SCSI_SENSE_BUFFERSIZE));
> +
> + scsicmd->scsi_done(scsicmd);
> + return 1;
> + }
> + }
This one, and numerous others are wrongly indented ... the continuation
lines should have an extra tab or be aligned under the opening bracket
for a function/macro. Indentation like this makes the file very hard to
read.
> diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
> --- a/drivers/scsi/aacraid/aacraid.h 2010-04-03 05:49:52.000000000 -0700
> +++ b/drivers/scsi/aacraid/aacraid.h 2010-04-07 14:17:18.409380952 -0700
> @@ -1,6 +1,7 @@
> #ifndef dprintk
> # define dprintk(x)
> #endif
> +#define AAC_DEBUG_INSTRUMENT_AIF_DELETE
What is this? It's not used anywhere else in the file (a leftover from
some removed debugging code?)
> @@ -1065,6 +1065,10 @@
> #define aac_adapter_ioremap(dev, size) \
> (dev)->a_ops.adapter_ioremap(dev, size)
>
> +#define aac_adapter_intr(dev) \
> + ((dev)->a_ops.adapter_intr((dev)->pdev->irq, \
> + (void *)dev))
> +
This is another very weird macro, but as far as I can tell, it's never
used either.
> --- a/drivers/scsi/aacraid/commctrl.c 2010-04-03 05:49:52.000000000
> -0700
> +++ b/drivers/scsi/aacraid/commctrl.c 2010-04-07 12:52:53.803318104
> -0700
> @@ -128,13 +128,13 @@
> retval =
> aac_fib_send(le16_to_cpu(kfib->header.Command), fibptr,
> le16_to_cpu(kfib->header.Size) ,
> FsaNormal,
> 1, 1, NULL, NULL);
> - if (retval) {
> - goto cleanup;
> - }
> if (aac_fib_complete(fibptr) != 0) {
> + if (!retval)
> retval = -EINVAL;
> - goto cleanup;
> + goto cleanup;
This looks wrong ... it's wrongly indented, but I'm assuming you meant
to remove the goto altogether so it gets swept up in the code below?
> }
> + if (retval)
> + goto cleanup;
> @@ -534,6 +532,13 @@
> "update mother board
> BIOS or consider utilizing one of\n"
> "the SAFE mode
> kernel options (acpi, apic etc)\n");
> }
> + spin_lock_irqsave(&fibptr->event_lock,
> + flags);
> + fibptr->done = 2;
> + spin_unlock_irqrestore(
> + &fibptr->event_lock,
> + flags);
So these locks ... even badly indented ... don't look necessary:
fibptr->done is a u32. On all architectures that's updated atomically.
This means that the check on fibptr->done always reads either the old
value or the new value ... there's no need to add extra locking unless
there are other values or states that need updating atomically.
> + dprintk((KERN_ERR "aacraid:
> sync. command timed out after 180 seconds\n"));
James
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 2/5] AACRAID driver update
2010-05-01 19:03 ` James Bottomley
2010-05-10 11:05 ` Rajashekhara, Mahesh
@ 2010-05-10 11:12 ` Rajashekhara, Mahesh
2010-05-17 2:54 ` James Bottomley
2010-05-10 11:17 ` [PATCH 3/5] " Rajashekhara, Mahesh
` (2 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Rajashekhara, Mahesh @ 2010-05-10 11:12 UTC (permalink / raw)
To: James Bottomley, linux-scsi@vger.kernel.org; +Cc: AACRAID
Hi All,
Below is the patch details:
Description:
------------
The default driver setting is "expose_physicals=0", which means raw physical drives are not exposed to OS.
If the user wants to expose connected physical drives, enable "expose_physicals" module parameter.
With the new JBOD firmware, physical drives are not available for "expose_physicals>0".
In function "aac_expose_phy_device", modified to reset the appropriate bit in the first byte of inquiry data.
This fix exposes the connected physical drives.
Please see patch below.
=====================
Mahesh
Linux Driver Development Engineer,
Adaptec ODC, Bangalore
This attached patch is against current scsi-misc-2.6.
ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments (inline gets damaged, please use attachment).
Signed-off-by: Mahesh Rajashekhara <aacraid@adaptec.com>
drivers/scsi/aacraid/aachba.c | 15 +++++++++++++++
drivers/scsi/aacraid/aacraid.h | 2 +-
2 files changed, 16 insertions(+), 1 deletion(-)
diff -ru a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
--- a/drivers/scsi/aacraid/aachba.c 2010-05-13 12:35:40.020534512 -0700
+++ b/drivers/scsi/aacraid/aachba.c 2010-05-13 12:38:50.289609216 -0700
@@ -328,6 +328,16 @@
return status;
}
+static void aac_expose_phy_device(struct scsi_cmnd *scsicmd)
+{
+ char inq_data;
+ scsi_sg_copy_to_buffer(scsicmd, &inq_data, sizeof(inq_data));
+ if ((inq_data & 0x20) && (inq_data & 0x1f) == TYPE_DISK) {
+ inq_data &= 0xdf;
+ scsi_sg_copy_from_buffer(scsicmd, &inq_data, sizeof(inq_data));
+ }
+}
+
/**
* aac_get_containers - list containers
* @common: adapter to probe
@@ -2573,6 +2583,11 @@
scsi_dma_unmap(scsicmd);
+ /* expose physical device if expose_physicald flag is on */
+ if (scsicmd->cmnd[0] == INQUIRY && !(scsicmd->cmnd[1] & 0x01)
+ && expose_physicals > 0)
+ aac_expose_phy_device(scsicmd);
+
/*
* First check the fib status
*/
diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
--- a/drivers/scsi/aacraid/aacraid.h 2010-05-13 12:35:40.020534512 -0700
+++ b/drivers/scsi/aacraid/aacraid.h 2010-05-13 12:37:21.823058176 -0700
@@ -12,7 +12,7 @@
*----------------------------------------------------------------------------*/
#ifndef AAC_DRIVER_BUILD
-# define AAC_DRIVER_BUILD 24702
+# define AAC_DRIVER_BUILD 26100
# define AAC_DRIVER_BRANCH "-ms"
#endif
#define MAXIMUM_NUM_CONTAINERS 32
Regards - Mahesh Rajashekhara
-----Original Message-----
From: James Bottomley [mailto:James.Bottomley@suse.de]
Sent: Sunday, May 02, 2010 12:33 AM
To: Rajashekhara, Mahesh
Cc: linux-scsi@vger.kernel.org; AACRAID
Subject: Re: AACRAID driver update patch version - 26400
On Wed, 2010-04-07 at 20:55 -0700, Rajashekhara, Mahesh wrote:
> (Resending in plain text as the earlier email got bounced)
>
> Hi All,
>
> We have prepared driver patch from latest driver source.
>
> We worked on a issue related to SMARTCTL([Bugme-new] [Bug 15623] New: doesn't allow SMART-passthrough on Adaptec Series 5 controller)
> and found the root cause of this problem. We have responded to this bug, stating that we will be releasing the driver patch for addressing this problem.
> Also, in our recent program releases we have addressed some more issues.
>
> Please look into the release notes.
Please, no. What I actually need is a set of patches (one per change or
feature) with a properly added description and signoff as described in
Documentation/SubmittingPatches.
> Please find the attachments.
> 1. aacraid-version-26400-040510-patch - Driver patch
> 2. aacraid-scsi-misc-2.6-warnings.txt - Warning message text
> 3. Release notes
So if I look at the Changelog it seems that there should be four
separate changes:
> AACRAID Driver for Linux - version 26400
> ----------------------------------------
>
> Addressed issues:
> ----------------
> 1. The default driver setting is "expose_physicals=0", which means raw
> physical drives not exposed to OS.
> If the user wants to expose connected physical drives, enable
> "expose_physicals" module parameter.
> With the new JBOD firmware, physical drives are not available for
> "expose_physicals>0".
> In function "aac_expose_phy_device", added to reset the appropriate
> bit in the first byte of inquiry data.
> This fix exposes the connected physical drives.
OK
> 2. Added support for handling ATA pass-through commands.
> When the "CC" bit is set by the host in ATA pass-through CDB,
> driver is supposed to return DID_OK
> When the "CC" bit is reset by the host, driver should return
> DID_ERROR
No, that's not correct. You should return a result code of
SAM_STAT_CHECK_CONDITION, with a driver byte of DID_OK. If you return
DID_ERROR, it will unconditionally trigger error handling and a retry.
> 3. Firmware exposes lesser container capacity than the actual one.
> It exposes [actaul size - hidden reserved space(10MB)] to the OS,
> IO's to the 10MB should be prohibhited from the Linux
> driver.
> When the IO's falls into hidden reserved space, driver internally
> sets sensekey to HARDWARE_ERROR and sends notification to the SCSI mid
> layer.
> Added "expose_hidden_space" flag, by default the fix will be
> executed.
> Only if the user sets "expose_hidden_space=1", user can access
> beyond the array reported size(hidden reserved space 10MB).
This doesn't exactly look like the right interface. Won't the user want
the hidden space exposed on a per array basis? In which case a sysfs
flag in the host attributes would be far more appropriate than a module
flag?
> 4. Based on the notification from firmware, the driver sets up
> SDV_OFFLINE flag for a deleted array and
> the SCSI mid layer comes to know the attached SCSI device has gone
> offline. But with
> this notification, scsi device entries are not being removed.
> Enabled "AAC_DEBUG_INSTRUMENT_AIF_DELETE" flag in the header file,
> with this flag the driver uses "scsi_remove_device".
> On the notification from firmware, the driver calls
> "scsi_remove_device" for the deleted array.
> This call not only informs the scsi device status to the SCSI mid
> layer but also
> removes corresponding scsi device entry from the linux
> sysfs.
OK, don't understand this. As noted below, the define
AAC_DEBUG_INSTRUMENT_AIF_DELETE is set by your patch, but never appears
at all in the driver ... is there missing code?
> Thanks & Regards,
> Mahesh
> Linux Driver Development Engineer,
> Adaptec ODC, Bangalore
> diff -ru a/drivers/scsi/aacraid/aachba.c
> b/drivers/scsi/aacraid/aachba.c
> --- a/drivers/scsi/aacraid/aachba.c 2010-04-03 05:49:52.000000000
> -0700
> +++ b/drivers/scsi/aacraid/aachba.c 2010-04-07 12:35:36.468017040
> -0700
> @@ -109,6 +109,16 @@
> #define BYTE2(x) (unsigned char)((x) >> 16)
> #define BYTE3(x) (unsigned char)((x) >> 24)
>
> +
> +/* ATA pass thru commands */
> +#ifndef ATA_12
> +#define ATA_12 0xa1 /* 12-byte pass-thru */
> +#endif
> +
> +#ifndef ATA_16
> +#define ATA_16 0x85 /* 16-byte pass-thru */
> +#endif
> +
This chunk is clearly wrong ... those defines are in include/scsi/scsi.h
> @@ -1647,6 +1671,27 @@
> count = (scsicmd->cmnd[7] << 8) | scsicmd->cmnd[8];
> break;
> }
> +
> + if (expose_hidden_space <= 0) {
> + if ((lba + count) > (dev->fsa_dev[scmd_id(scsicmd)].size)) {
> + int cid = scmd_id(scsicmd);
> + dprintk((KERN_DEBUG "aacraid: Illegal lba\n"));
> + scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
> + SAM_STAT_CHECK_CONDITION;
> + set_sense(&dev->fsa_dev[cid].sense_data,
> + HARDWARE_ERROR,
> + SENCODE_INTERNAL_TARGET_FAILURE,
> + ASENCODE_INTERNAL_TARGET_FAILURE, 0, 0);
> + memcpy(scsicmd->sense_buffer,
> + &dev->fsa_dev[cid].sense_data,
> + min_t(size_t,
> + sizeof(dev->fsa_dev[cid].sense_data),
> + SCSI_SENSE_BUFFERSIZE));
> + scsicmd->scsi_done(scsicmd);
> + return 1;
> + }
> + }
> +
This chunk is correctly indented, that's great.
> dprintk((KERN_DEBUG "aac_read[cpu %d]: lba = %llu, t = %ld.\n",
> smp_processor_id(), (unsigned long long)lba, jiffies));
> if (aac_adapter_bounds(dev,scsicmd,lba))
> @@ -1727,6 +1772,26 @@
> count = (scsicmd->cmnd[7] << 8) | scsicmd->cmnd[8];
> fua = scsicmd->cmnd[1] & 0x8;
> }
> +
> + if (expose_hidden_space <= 0) {
> + if ((lba + count) > (dev->fsa_dev[scmd_id(scsicmd)].size)) {
> + int cid = scmd_id(scsicmd);
> + dprintk((KERN_DEBUG "aacraid: Illegal lba\n"));
> + scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
> + SAM_STAT_CHECK_CONDITION;
> + set_sense(&dev->fsa_dev[cid].sense_data,
> + HARDWARE_ERROR, SENCODE_INTERNAL_TARGET_FAILURE,
> + ASENCODE_INTERNAL_TARGET_FAILURE, 0, 0);
> + memcpy(scsicmd->sense_buffer,
> + &dev->fsa_dev[cid].sense_data,
> + min_t(size_t,
> + sizeof(dev->fsa_dev[cid].sense_data),
> + SCSI_SENSE_BUFFERSIZE));
> +
> + scsicmd->scsi_done(scsicmd);
> + return 1;
> + }
> + }
This one, and numerous others are wrongly indented ... the continuation
lines should have an extra tab or be aligned under the opening bracket
for a function/macro. Indentation like this makes the file very hard to
read.
> diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
> --- a/drivers/scsi/aacraid/aacraid.h 2010-04-03 05:49:52.000000000 -0700
> +++ b/drivers/scsi/aacraid/aacraid.h 2010-04-07 14:17:18.409380952 -0700
> @@ -1,6 +1,7 @@
> #ifndef dprintk
> # define dprintk(x)
> #endif
> +#define AAC_DEBUG_INSTRUMENT_AIF_DELETE
What is this? It's not used anywhere else in the file (a leftover from
some removed debugging code?)
> @@ -1065,6 +1065,10 @@
> #define aac_adapter_ioremap(dev, size) \
> (dev)->a_ops.adapter_ioremap(dev, size)
>
> +#define aac_adapter_intr(dev) \
> + ((dev)->a_ops.adapter_intr((dev)->pdev->irq, \
> + (void *)dev))
> +
This is another very weird macro, but as far as I can tell, it's never
used either.
> --- a/drivers/scsi/aacraid/commctrl.c 2010-04-03 05:49:52.000000000
> -0700
> +++ b/drivers/scsi/aacraid/commctrl.c 2010-04-07 12:52:53.803318104
> -0700
> @@ -128,13 +128,13 @@
> retval =
> aac_fib_send(le16_to_cpu(kfib->header.Command), fibptr,
> le16_to_cpu(kfib->header.Size) ,
> FsaNormal,
> 1, 1, NULL, NULL);
> - if (retval) {
> - goto cleanup;
> - }
> if (aac_fib_complete(fibptr) != 0) {
> + if (!retval)
> retval = -EINVAL;
> - goto cleanup;
> + goto cleanup;
This looks wrong ... it's wrongly indented, but I'm assuming you meant
to remove the goto altogether so it gets swept up in the code below?
> }
> + if (retval)
> + goto cleanup;
> @@ -534,6 +532,13 @@
> "update mother board
> BIOS or consider utilizing one of\n"
> "the SAFE mode
> kernel options (acpi, apic etc)\n");
> }
> + spin_lock_irqsave(&fibptr->event_lock,
> + flags);
> + fibptr->done = 2;
> + spin_unlock_irqrestore(
> + &fibptr->event_lock,
> + flags);
So these locks ... even badly indented ... don't look necessary:
fibptr->done is a u32. On all architectures that's updated atomically.
This means that the check on fibptr->done always reads either the old
value or the new value ... there's no need to add extra locking unless
there are other values or states that need updating atomically.
> + dprintk((KERN_ERR "aacraid:
> sync. command timed out after 180 seconds\n"));
James
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 2/5] AACRAID driver update
2010-05-10 11:12 ` [PATCH 2/5] AACRAID driver update Rajashekhara, Mahesh
@ 2010-05-17 2:54 ` James Bottomley
0 siblings, 0 replies; 9+ messages in thread
From: James Bottomley @ 2010-05-17 2:54 UTC (permalink / raw)
To: Rajashekhara, Mahesh; +Cc: linux-scsi@vger.kernel.org, AACRAID
On Mon, 2010-05-10 at 04:12 -0700, Rajashekhara, Mahesh wrote:
This subject line [PATCH X/5] AACRAID driver update isn't descriptive of
the patch ... I interpolated proper subject lines from the description.
> ObligatoryDisclaimer: Please accept my condolences regarding Outlook's
> handling of patch attachments (inline gets damaged, please use
> attachment).
The inline seemed to work mostly (lucky, because there are no
attachments).
> diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
> --- a/drivers/scsi/aacraid/aacraid.h 2010-05-13 12:35:40.020534512 -0700
> +++ b/drivers/scsi/aacraid/aacraid.h 2010-05-13 12:37:21.823058176 -0700
> @@ -12,7 +12,7 @@
> *----------------------------------------------------------------------------*/
>
> #ifndef AAC_DRIVER_BUILD
> -# define AAC_DRIVER_BUILD 24702
All of your patches remove this same version number. That makes each of
the patches after the first reject here when applied in sequence. I
just stripped this out and updated the version number in the final
patch.
James
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/5] AACRAID driver update
2010-05-01 19:03 ` James Bottomley
2010-05-10 11:05 ` Rajashekhara, Mahesh
2010-05-10 11:12 ` [PATCH 2/5] AACRAID driver update Rajashekhara, Mahesh
@ 2010-05-10 11:17 ` Rajashekhara, Mahesh
2010-05-10 11:24 ` [PATCH 4/5] " Rajashekhara, Mahesh
2010-05-10 11:29 ` [PATCH 5/5] " Rajashekhara, Mahesh
4 siblings, 0 replies; 9+ messages in thread
From: Rajashekhara, Mahesh @ 2010-05-10 11:17 UTC (permalink / raw)
To: James Bottomley, linux-scsi@vger.kernel.org; +Cc: AACRAID
Hi All,
Below is the patch details:
Description:
------------
Added support for handling ATA pass-through commands.
There are two conditions for ATA pass thru command that falls into 'SRB_STATUS_ERROR' condition.
1. When the "CC" bit is set by the host in ATA pass-through CDB
- Even for the successful completion, SCSI target shall generate check condition.
- Driver returns a result code of SAM_STAT_CHECK_CONDITION, with a driver byte of DID_OK to the mid layer.
Below is the snippet of existing code which fills a result code of SAM_STAT_CHECK_CONDITION:
***********************************
if (le32_to_cpu(srbreply->scsi_status) == SAM_STAT_CHECK_CONDITION) {
int len;
scsicmd->result |= SAM_STAT_CHECK_CONDITION;
..........
************************************
2. When the "CC" bit is reset by the host and if SCSI target generates a check condition when an error occurs.
- Driver returns a result code of SAM_STAT_CHECK_CONDITION, with a driver byte of DID_ERROR to the mid layer.
Please see patch below.
=====================
Mahesh
Linux Driver Development Engineer,
Adaptec ODC, Bangalore
This attached patch is against current scsi-misc-2.6.
ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments (inline gets damaged, please use attachment).
Signed-off-by: Mahesh Rajashekhara <aacraid@adaptec.com>
drivers/scsi/aacraid/aachba.c | 18 ++++++++++++++++--
drivers/scsi/aacraid/aacraid.h | 2 +-
2 files changed, 17 insertions(+), 3 deletions(-)
diff -ru a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
--- a/drivers/scsi/aacraid/aachba.c 2010-05-13 12:26:10.043184240 -0700
+++ b/drivers/scsi/aacraid/aachba.c 2010-05-13 12:32:26.422965816 -0700
@@ -2678,8 +2678,22 @@
scsicmd->cmnd[0],
le32_to_cpu(srbreply->scsi_status));
#endif
- scsicmd->result = DID_ERROR << 16 | COMMAND_COMPLETE << 8;
- break;
+ if ((scsicmd->cmnd[0] == ATA_12)
+ || (scsicmd->cmnd[0] == ATA_16)) {
+ if (scsicmd->cmnd[2] & (0x01 << 5)) {
+ scsicmd->result = DID_OK << 16
+ | COMMAND_COMPLETE << 8;
+ break;
+ } else {
+ scsicmd->result = DID_ERROR << 16
+ | COMMAND_COMPLETE << 8;
+ break;
+ }
+ } else {
+ scsicmd->result = DID_ERROR << 16
+ | COMMAND_COMPLETE << 8;
+ break;
+ }
}
if (le32_to_cpu(srbreply->scsi_status) == SAM_STAT_CHECK_CONDITION) {
int len;
diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
--- a/drivers/scsi/aacraid/aacraid.h 2010-05-13 12:26:10.044184088 -0700
+++ b/drivers/scsi/aacraid/aacraid.h 2010-05-13 12:28:13.932350224 -0700
@@ -12,7 +12,7 @@
*----------------------------------------------------------------------------*/
#ifndef AAC_DRIVER_BUILD
-# define AAC_DRIVER_BUILD 24702
+# define AAC_DRIVER_BUILD 26200
# define AAC_DRIVER_BRANCH "-ms"
#endif
#define MAXIMUM_NUM_CONTAINERS 32
Regards - Mahesh Rajashekhara
-----Original Message-----
From: James Bottomley [mailto:James.Bottomley@suse.de]
Sent: Sunday, May 02, 2010 12:33 AM
To: Rajashekhara, Mahesh
Cc: linux-scsi@vger.kernel.org; AACRAID
Subject: Re: AACRAID driver update patch version - 26400
On Wed, 2010-04-07 at 20:55 -0700, Rajashekhara, Mahesh wrote:
> (Resending in plain text as the earlier email got bounced)
>
> Hi All,
>
> We have prepared driver patch from latest driver source.
>
> We worked on a issue related to SMARTCTL([Bugme-new] [Bug 15623] New: doesn't allow SMART-passthrough on Adaptec Series 5 controller)
> and found the root cause of this problem. We have responded to this bug, stating that we will be releasing the driver patch for addressing this problem.
> Also, in our recent program releases we have addressed some more issues.
>
> Please look into the release notes.
Please, no. What I actually need is a set of patches (one per change or
feature) with a properly added description and signoff as described in
Documentation/SubmittingPatches.
> Please find the attachments.
> 1. aacraid-version-26400-040510-patch - Driver patch
> 2. aacraid-scsi-misc-2.6-warnings.txt - Warning message text
> 3. Release notes
So if I look at the Changelog it seems that there should be four
separate changes:
> AACRAID Driver for Linux - version 26400
> ----------------------------------------
>
> Addressed issues:
> ----------------
> 1. The default driver setting is "expose_physicals=0", which means raw
> physical drives not exposed to OS.
> If the user wants to expose connected physical drives, enable
> "expose_physicals" module parameter.
> With the new JBOD firmware, physical drives are not available for
> "expose_physicals>0".
> In function "aac_expose_phy_device", added to reset the appropriate
> bit in the first byte of inquiry data.
> This fix exposes the connected physical drives.
OK
> 2. Added support for handling ATA pass-through commands.
> When the "CC" bit is set by the host in ATA pass-through CDB,
> driver is supposed to return DID_OK
> When the "CC" bit is reset by the host, driver should return
> DID_ERROR
No, that's not correct. You should return a result code of
SAM_STAT_CHECK_CONDITION, with a driver byte of DID_OK. If you return
DID_ERROR, it will unconditionally trigger error handling and a retry.
> 3. Firmware exposes lesser container capacity than the actual one.
> It exposes [actaul size - hidden reserved space(10MB)] to the OS,
> IO's to the 10MB should be prohibhited from the Linux
> driver.
> When the IO's falls into hidden reserved space, driver internally
> sets sensekey to HARDWARE_ERROR and sends notification to the SCSI mid
> layer.
> Added "expose_hidden_space" flag, by default the fix will be
> executed.
> Only if the user sets "expose_hidden_space=1", user can access
> beyond the array reported size(hidden reserved space 10MB).
This doesn't exactly look like the right interface. Won't the user want
the hidden space exposed on a per array basis? In which case a sysfs
flag in the host attributes would be far more appropriate than a module
flag?
> 4. Based on the notification from firmware, the driver sets up
> SDV_OFFLINE flag for a deleted array and
> the SCSI mid layer comes to know the attached SCSI device has gone
> offline. But with
> this notification, scsi device entries are not being removed.
> Enabled "AAC_DEBUG_INSTRUMENT_AIF_DELETE" flag in the header file,
> with this flag the driver uses "scsi_remove_device".
> On the notification from firmware, the driver calls
> "scsi_remove_device" for the deleted array.
> This call not only informs the scsi device status to the SCSI mid
> layer but also
> removes corresponding scsi device entry from the linux
> sysfs.
OK, don't understand this. As noted below, the define
AAC_DEBUG_INSTRUMENT_AIF_DELETE is set by your patch, but never appears
at all in the driver ... is there missing code?
> Thanks & Regards,
> Mahesh
> Linux Driver Development Engineer,
> Adaptec ODC, Bangalore
> diff -ru a/drivers/scsi/aacraid/aachba.c
> b/drivers/scsi/aacraid/aachba.c
> --- a/drivers/scsi/aacraid/aachba.c 2010-04-03 05:49:52.000000000
> -0700
> +++ b/drivers/scsi/aacraid/aachba.c 2010-04-07 12:35:36.468017040
> -0700
> @@ -109,6 +109,16 @@
> #define BYTE2(x) (unsigned char)((x) >> 16)
> #define BYTE3(x) (unsigned char)((x) >> 24)
>
> +
> +/* ATA pass thru commands */
> +#ifndef ATA_12
> +#define ATA_12 0xa1 /* 12-byte pass-thru */
> +#endif
> +
> +#ifndef ATA_16
> +#define ATA_16 0x85 /* 16-byte pass-thru */
> +#endif
> +
This chunk is clearly wrong ... those defines are in include/scsi/scsi.h
> @@ -1647,6 +1671,27 @@
> count = (scsicmd->cmnd[7] << 8) | scsicmd->cmnd[8];
> break;
> }
> +
> + if (expose_hidden_space <= 0) {
> + if ((lba + count) > (dev->fsa_dev[scmd_id(scsicmd)].size)) {
> + int cid = scmd_id(scsicmd);
> + dprintk((KERN_DEBUG "aacraid: Illegal lba\n"));
> + scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
> + SAM_STAT_CHECK_CONDITION;
> + set_sense(&dev->fsa_dev[cid].sense_data,
> + HARDWARE_ERROR,
> + SENCODE_INTERNAL_TARGET_FAILURE,
> + ASENCODE_INTERNAL_TARGET_FAILURE, 0, 0);
> + memcpy(scsicmd->sense_buffer,
> + &dev->fsa_dev[cid].sense_data,
> + min_t(size_t,
> + sizeof(dev->fsa_dev[cid].sense_data),
> + SCSI_SENSE_BUFFERSIZE));
> + scsicmd->scsi_done(scsicmd);
> + return 1;
> + }
> + }
> +
This chunk is correctly indented, that's great.
> dprintk((KERN_DEBUG "aac_read[cpu %d]: lba = %llu, t = %ld.\n",
> smp_processor_id(), (unsigned long long)lba, jiffies));
> if (aac_adapter_bounds(dev,scsicmd,lba))
> @@ -1727,6 +1772,26 @@
> count = (scsicmd->cmnd[7] << 8) | scsicmd->cmnd[8];
> fua = scsicmd->cmnd[1] & 0x8;
> }
> +
> + if (expose_hidden_space <= 0) {
> + if ((lba + count) > (dev->fsa_dev[scmd_id(scsicmd)].size)) {
> + int cid = scmd_id(scsicmd);
> + dprintk((KERN_DEBUG "aacraid: Illegal lba\n"));
> + scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
> + SAM_STAT_CHECK_CONDITION;
> + set_sense(&dev->fsa_dev[cid].sense_data,
> + HARDWARE_ERROR, SENCODE_INTERNAL_TARGET_FAILURE,
> + ASENCODE_INTERNAL_TARGET_FAILURE, 0, 0);
> + memcpy(scsicmd->sense_buffer,
> + &dev->fsa_dev[cid].sense_data,
> + min_t(size_t,
> + sizeof(dev->fsa_dev[cid].sense_data),
> + SCSI_SENSE_BUFFERSIZE));
> +
> + scsicmd->scsi_done(scsicmd);
> + return 1;
> + }
> + }
This one, and numerous others are wrongly indented ... the continuation
lines should have an extra tab or be aligned under the opening bracket
for a function/macro. Indentation like this makes the file very hard to
read.
> diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
> --- a/drivers/scsi/aacraid/aacraid.h 2010-04-03 05:49:52.000000000 -0700
> +++ b/drivers/scsi/aacraid/aacraid.h 2010-04-07 14:17:18.409380952 -0700
> @@ -1,6 +1,7 @@
> #ifndef dprintk
> # define dprintk(x)
> #endif
> +#define AAC_DEBUG_INSTRUMENT_AIF_DELETE
What is this? It's not used anywhere else in the file (a leftover from
some removed debugging code?)
> @@ -1065,6 +1065,10 @@
> #define aac_adapter_ioremap(dev, size) \
> (dev)->a_ops.adapter_ioremap(dev, size)
>
> +#define aac_adapter_intr(dev) \
> + ((dev)->a_ops.adapter_intr((dev)->pdev->irq, \
> + (void *)dev))
> +
This is another very weird macro, but as far as I can tell, it's never
used either.
> --- a/drivers/scsi/aacraid/commctrl.c 2010-04-03 05:49:52.000000000
> -0700
> +++ b/drivers/scsi/aacraid/commctrl.c 2010-04-07 12:52:53.803318104
> -0700
> @@ -128,13 +128,13 @@
> retval =
> aac_fib_send(le16_to_cpu(kfib->header.Command), fibptr,
> le16_to_cpu(kfib->header.Size) ,
> FsaNormal,
> 1, 1, NULL, NULL);
> - if (retval) {
> - goto cleanup;
> - }
> if (aac_fib_complete(fibptr) != 0) {
> + if (!retval)
> retval = -EINVAL;
> - goto cleanup;
> + goto cleanup;
This looks wrong ... it's wrongly indented, but I'm assuming you meant
to remove the goto altogether so it gets swept up in the code below?
> }
> + if (retval)
> + goto cleanup;
> @@ -534,6 +532,13 @@
> "update mother board
> BIOS or consider utilizing one of\n"
> "the SAFE mode
> kernel options (acpi, apic etc)\n");
> }
> + spin_lock_irqsave(&fibptr->event_lock,
> + flags);
> + fibptr->done = 2;
> + spin_unlock_irqrestore(
> + &fibptr->event_lock,
> + flags);
So these locks ... even badly indented ... don't look necessary:
fibptr->done is a u32. On all architectures that's updated atomically.
This means that the check on fibptr->done always reads either the old
value or the new value ... there's no need to add extra locking unless
there are other values or states that need updating atomically.
> + dprintk((KERN_ERR "aacraid:
> sync. command timed out after 180 seconds\n"));
James
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 4/5] AACRAID driver update
2010-05-01 19:03 ` James Bottomley
` (2 preceding siblings ...)
2010-05-10 11:17 ` [PATCH 3/5] " Rajashekhara, Mahesh
@ 2010-05-10 11:24 ` Rajashekhara, Mahesh
2010-05-17 2:59 ` James Bottomley
2010-05-10 11:29 ` [PATCH 5/5] " Rajashekhara, Mahesh
4 siblings, 1 reply; 9+ messages in thread
From: Rajashekhara, Mahesh @ 2010-05-10 11:24 UTC (permalink / raw)
To: James Bottomley, linux-scsi@vger.kernel.org; +Cc: AACRAID
Hi All,
Below is the patch details:
Problem description:
--------------------
The issue reported by one of the customer was able to read LBA beyond the array
reported size with "sg_read" utility. If N is the last block address reported, then should not
be able to read past N, i.e. N+1. But in their case, reported last LBA=143134719.
So should not have been able to read with LBA=143134720, but it is read
without failure, which means reported size to the OS is not correct and is less than the actual last block address.
Solution:
---------
Firmware layer exposes lesser container capacity than the actual one. It exposes [Actual size - Spitfire space(10MB)]
to the OS, IO's to the 10MB should be prohibited from the Linux driver. Driver checks LBA boundary, if its greater
than the array reported size then sets sensekey to HARDWARE_ERROR and sends the notification to the MID layer.
Please see patch below.
=====================
Mahesh
Linux Driver Development Engineer,
Adaptec ODC, Bangalore
This attached patch is against current scsi-misc-2.6.
ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments.
Signed-off-by: Mahesh Rajashekhara <aacraid@adaptec.com>
drivers/scsi/aacraid/aachba.c | 34 ++++++++++++++++++++++++++++++++++
drivers/scsi/aacraid/aacraid.h | 2 +-
2 files changed, 35 insertions(+), 1 deletion(-)
diff -ru a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
--- a/drivers/scsi/aacraid/aachba.c 2010-05-13 12:50:36.684220912 -0700
+++ b/drivers/scsi/aacraid/aachba.c 2010-05-13 12:59:59.412673184 -0700
@@ -1598,6 +1598,7 @@
int status;
struct aac_dev *dev;
struct fib * cmd_fibcontext;
+ int cid;
dev = (struct aac_dev *)scsicmd->device->host->hostdata;
/*
@@ -1647,6 +1648,22 @@
count = (scsicmd->cmnd[7] << 8) | scsicmd->cmnd[8];
break;
}
+
+ if ((lba + count) > (dev->fsa_dev[scmd_id(scsicmd)].size)) {
+ cid = scmd_id(scsicmd);
+ dprintk((KERN_DEBUG "aacraid: Illegal lba\n"));
+ scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
+ SAM_STAT_CHECK_CONDITION;
+ set_sense(&dev->fsa_dev[cid].sense_data,
+ HARDWARE_ERROR, SENCODE_INTERNAL_TARGET_FAILURE,
+ ASENCODE_INTERNAL_TARGET_FAILURE, 0, 0);
+ memcpy(scsicmd->sense_buffer, &dev->fsa_dev[cid].sense_data,
+ min_t(size_t, sizeof(dev->fsa_dev[cid].sense_data),
+ SCSI_SENSE_BUFFERSIZE));
+ scsicmd->scsi_done(scsicmd);
+ return 1;
+ }
+
dprintk((KERN_DEBUG "aac_read[cpu %d]: lba = %llu, t = %ld.\n",
smp_processor_id(), (unsigned long long)lba, jiffies));
if (aac_adapter_bounds(dev,scsicmd,lba))
@@ -1688,6 +1705,7 @@
int status;
struct aac_dev *dev;
struct fib * cmd_fibcontext;
+ int cid;
dev = (struct aac_dev *)scsicmd->device->host->hostdata;
/*
@@ -1727,6 +1745,22 @@
count = (scsicmd->cmnd[7] << 8) | scsicmd->cmnd[8];
fua = scsicmd->cmnd[1] & 0x8;
}
+
+ if ((lba + count) > (dev->fsa_dev[scmd_id(scsicmd)].size)) {
+ cid = scmd_id(scsicmd);
+ dprintk((KERN_DEBUG "aacraid: Illegal lba\n"));
+ scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
+ SAM_STAT_CHECK_CONDITION;
+ set_sense(&dev->fsa_dev[cid].sense_data,
+ HARDWARE_ERROR, SENCODE_INTERNAL_TARGET_FAILURE,
+ ASENCODE_INTERNAL_TARGET_FAILURE, 0, 0);
+ memcpy(scsicmd->sense_buffer, &dev->fsa_dev[cid].sense_data,
+ min_t(size_t, sizeof(dev->fsa_dev[cid].sense_data),
+ SCSI_SENSE_BUFFERSIZE));
+ scsicmd->scsi_done(scsicmd);
+ return 1;
+ }
+
dprintk((KERN_DEBUG "aac_write[cpu %d]: lba = %llu, t = %ld.\n",
smp_processor_id(), (unsigned long long)lba, jiffies));
if (aac_adapter_bounds(dev,scsicmd,lba))
diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
--- a/drivers/scsi/aacraid/aacraid.h 2010-05-13 12:50:36.685220760 -0700
+++ b/drivers/scsi/aacraid/aacraid.h 2010-05-13 12:52:10.989884272 -0700
@@ -12,7 +12,7 @@
*----------------------------------------------------------------------------*/
#ifndef AAC_DRIVER_BUILD
-# define AAC_DRIVER_BUILD 24702
+# define AAC_DRIVER_BUILD 26300
# define AAC_DRIVER_BRANCH "-ms"
#endif
#define MAXIMUM_NUM_CONTAINERS 32
Regards - Mahesh Rajashekhara
-----Original Message-----
From: James Bottomley [mailto:James.Bottomley@suse.de]
Sent: Sunday, May 02, 2010 12:33 AM
To: Rajashekhara, Mahesh
Cc: linux-scsi@vger.kernel.org; AACRAID
Subject: Re: AACRAID driver update patch version - 26400
On Wed, 2010-04-07 at 20:55 -0700, Rajashekhara, Mahesh wrote:
> (Resending in plain text as the earlier email got bounced)
>
> Hi All,
>
> We have prepared driver patch from latest driver source.
>
> We worked on a issue related to SMARTCTL([Bugme-new] [Bug 15623] New: doesn't allow SMART-passthrough on Adaptec Series 5 controller)
> and found the root cause of this problem. We have responded to this bug, stating that we will be releasing the driver patch for addressing this problem.
> Also, in our recent program releases we have addressed some more issues.
>
> Please look into the release notes.
Please, no. What I actually need is a set of patches (one per change or
feature) with a properly added description and signoff as described in
Documentation/SubmittingPatches.
> Please find the attachments.
> 1. aacraid-version-26400-040510-patch - Driver patch
> 2. aacraid-scsi-misc-2.6-warnings.txt - Warning message text
> 3. Release notes
So if I look at the Changelog it seems that there should be four
separate changes:
> AACRAID Driver for Linux - version 26400
> ----------------------------------------
>
> Addressed issues:
> ----------------
> 1. The default driver setting is "expose_physicals=0", which means raw
> physical drives not exposed to OS.
> If the user wants to expose connected physical drives, enable
> "expose_physicals" module parameter.
> With the new JBOD firmware, physical drives are not available for
> "expose_physicals>0".
> In function "aac_expose_phy_device", added to reset the appropriate
> bit in the first byte of inquiry data.
> This fix exposes the connected physical drives.
OK
> 2. Added support for handling ATA pass-through commands.
> When the "CC" bit is set by the host in ATA pass-through CDB,
> driver is supposed to return DID_OK
> When the "CC" bit is reset by the host, driver should return
> DID_ERROR
No, that's not correct. You should return a result code of
SAM_STAT_CHECK_CONDITION, with a driver byte of DID_OK. If you return
DID_ERROR, it will unconditionally trigger error handling and a retry.
> 3. Firmware exposes lesser container capacity than the actual one.
> It exposes [actaul size - hidden reserved space(10MB)] to the OS,
> IO's to the 10MB should be prohibhited from the Linux
> driver.
> When the IO's falls into hidden reserved space, driver internally
> sets sensekey to HARDWARE_ERROR and sends notification to the SCSI mid
> layer.
> Added "expose_hidden_space" flag, by default the fix will be
> executed.
> Only if the user sets "expose_hidden_space=1", user can access
> beyond the array reported size(hidden reserved space 10MB).
This doesn't exactly look like the right interface. Won't the user want
the hidden space exposed on a per array basis? In which case a sysfs
flag in the host attributes would be far more appropriate than a module
flag?
> 4. Based on the notification from firmware, the driver sets up
> SDV_OFFLINE flag for a deleted array and
> the SCSI mid layer comes to know the attached SCSI device has gone
> offline. But with
> this notification, scsi device entries are not being removed.
> Enabled "AAC_DEBUG_INSTRUMENT_AIF_DELETE" flag in the header file,
> with this flag the driver uses "scsi_remove_device".
> On the notification from firmware, the driver calls
> "scsi_remove_device" for the deleted array.
> This call not only informs the scsi device status to the SCSI mid
> layer but also
> removes corresponding scsi device entry from the linux
> sysfs.
OK, don't understand this. As noted below, the define
AAC_DEBUG_INSTRUMENT_AIF_DELETE is set by your patch, but never appears
at all in the driver ... is there missing code?
> Thanks & Regards,
> Mahesh
> Linux Driver Development Engineer,
> Adaptec ODC, Bangalore
> diff -ru a/drivers/scsi/aacraid/aachba.c
> b/drivers/scsi/aacraid/aachba.c
> --- a/drivers/scsi/aacraid/aachba.c 2010-04-03 05:49:52.000000000
> -0700
> +++ b/drivers/scsi/aacraid/aachba.c 2010-04-07 12:35:36.468017040
> -0700
> @@ -109,6 +109,16 @@
> #define BYTE2(x) (unsigned char)((x) >> 16)
> #define BYTE3(x) (unsigned char)((x) >> 24)
>
> +
> +/* ATA pass thru commands */
> +#ifndef ATA_12
> +#define ATA_12 0xa1 /* 12-byte pass-thru */
> +#endif
> +
> +#ifndef ATA_16
> +#define ATA_16 0x85 /* 16-byte pass-thru */
> +#endif
> +
This chunk is clearly wrong ... those defines are in include/scsi/scsi.h
> @@ -1647,6 +1671,27 @@
> count = (scsicmd->cmnd[7] << 8) | scsicmd->cmnd[8];
> break;
> }
> +
> + if (expose_hidden_space <= 0) {
> + if ((lba + count) > (dev->fsa_dev[scmd_id(scsicmd)].size)) {
> + int cid = scmd_id(scsicmd);
> + dprintk((KERN_DEBUG "aacraid: Illegal lba\n"));
> + scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
> + SAM_STAT_CHECK_CONDITION;
> + set_sense(&dev->fsa_dev[cid].sense_data,
> + HARDWARE_ERROR,
> + SENCODE_INTERNAL_TARGET_FAILURE,
> + ASENCODE_INTERNAL_TARGET_FAILURE, 0, 0);
> + memcpy(scsicmd->sense_buffer,
> + &dev->fsa_dev[cid].sense_data,
> + min_t(size_t,
> + sizeof(dev->fsa_dev[cid].sense_data),
> + SCSI_SENSE_BUFFERSIZE));
> + scsicmd->scsi_done(scsicmd);
> + return 1;
> + }
> + }
> +
This chunk is correctly indented, that's great.
> dprintk((KERN_DEBUG "aac_read[cpu %d]: lba = %llu, t = %ld.\n",
> smp_processor_id(), (unsigned long long)lba, jiffies));
> if (aac_adapter_bounds(dev,scsicmd,lba))
> @@ -1727,6 +1772,26 @@
> count = (scsicmd->cmnd[7] << 8) | scsicmd->cmnd[8];
> fua = scsicmd->cmnd[1] & 0x8;
> }
> +
> + if (expose_hidden_space <= 0) {
> + if ((lba + count) > (dev->fsa_dev[scmd_id(scsicmd)].size)) {
> + int cid = scmd_id(scsicmd);
> + dprintk((KERN_DEBUG "aacraid: Illegal lba\n"));
> + scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
> + SAM_STAT_CHECK_CONDITION;
> + set_sense(&dev->fsa_dev[cid].sense_data,
> + HARDWARE_ERROR, SENCODE_INTERNAL_TARGET_FAILURE,
> + ASENCODE_INTERNAL_TARGET_FAILURE, 0, 0);
> + memcpy(scsicmd->sense_buffer,
> + &dev->fsa_dev[cid].sense_data,
> + min_t(size_t,
> + sizeof(dev->fsa_dev[cid].sense_data),
> + SCSI_SENSE_BUFFERSIZE));
> +
> + scsicmd->scsi_done(scsicmd);
> + return 1;
> + }
> + }
This one, and numerous others are wrongly indented ... the continuation
lines should have an extra tab or be aligned under the opening bracket
for a function/macro. Indentation like this makes the file very hard to
read.
> diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
> --- a/drivers/scsi/aacraid/aacraid.h 2010-04-03 05:49:52.000000000 -0700
> +++ b/drivers/scsi/aacraid/aacraid.h 2010-04-07 14:17:18.409380952 -0700
> @@ -1,6 +1,7 @@
> #ifndef dprintk
> # define dprintk(x)
> #endif
> +#define AAC_DEBUG_INSTRUMENT_AIF_DELETE
What is this? It's not used anywhere else in the file (a leftover from
some removed debugging code?)
> @@ -1065,6 +1065,10 @@
> #define aac_adapter_ioremap(dev, size) \
> (dev)->a_ops.adapter_ioremap(dev, size)
>
> +#define aac_adapter_intr(dev) \
> + ((dev)->a_ops.adapter_intr((dev)->pdev->irq, \
> + (void *)dev))
> +
This is another very weird macro, but as far as I can tell, it's never
used either.
> --- a/drivers/scsi/aacraid/commctrl.c 2010-04-03 05:49:52.000000000
> -0700
> +++ b/drivers/scsi/aacraid/commctrl.c 2010-04-07 12:52:53.803318104
> -0700
> @@ -128,13 +128,13 @@
> retval =
> aac_fib_send(le16_to_cpu(kfib->header.Command), fibptr,
> le16_to_cpu(kfib->header.Size) ,
> FsaNormal,
> 1, 1, NULL, NULL);
> - if (retval) {
> - goto cleanup;
> - }
> if (aac_fib_complete(fibptr) != 0) {
> + if (!retval)
> retval = -EINVAL;
> - goto cleanup;
> + goto cleanup;
This looks wrong ... it's wrongly indented, but I'm assuming you meant
to remove the goto altogether so it gets swept up in the code below?
> }
> + if (retval)
> + goto cleanup;
> @@ -534,6 +532,13 @@
> "update mother board
> BIOS or consider utilizing one of\n"
> "the SAFE mode
> kernel options (acpi, apic etc)\n");
> }
> + spin_lock_irqsave(&fibptr->event_lock,
> + flags);
> + fibptr->done = 2;
> + spin_unlock_irqrestore(
> + &fibptr->event_lock,
> + flags);
So these locks ... even badly indented ... don't look necessary:
fibptr->done is a u32. On all architectures that's updated atomically.
This means that the check on fibptr->done always reads either the old
value or the new value ... there's no need to add extra locking unless
there are other values or states that need updating atomically.
> + dprintk((KERN_ERR "aacraid:
> sync. command timed out after 180 seconds\n"));
James
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 4/5] AACRAID driver update
2010-05-10 11:24 ` [PATCH 4/5] " Rajashekhara, Mahesh
@ 2010-05-17 2:59 ` James Bottomley
0 siblings, 0 replies; 9+ messages in thread
From: James Bottomley @ 2010-05-17 2:59 UTC (permalink / raw)
To: Rajashekhara, Mahesh; +Cc: linux-scsi@vger.kernel.org, AACRAID
On Mon, 2010-05-10 at 04:24 -0700, Rajashekhara, Mahesh wrote:
> diff -ru a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
> --- a/drivers/scsi/aacraid/aachba.c 2010-05-13 12:50:36.684220912 -0700
> +++ b/drivers/scsi/aacraid/aachba.c 2010-05-13 12:59:59.412673184 -0700
> @@ -1598,6 +1598,7 @@
> int status;
> struct aac_dev *dev;
> struct fib * cmd_fibcontext;
> + int cid;
>
> dev = (struct aac_dev *)scsicmd->device->host->hostdata;
> /*
> @@ -1647,6 +1648,22 @@
> count = (scsicmd->cmnd[7] << 8) | scsicmd->cmnd[8];
> break;
> }
> +
> + if ((lba + count) > (dev->fsa_dev[scmd_id(scsicmd)].size)) {
> + cid = scmd_id(scsicmd);
> + dprintk((KERN_DEBUG "aacraid: Illegal lba\n"));
> + scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
> + SAM_STAT_CHECK_CONDITION;
> + set_sense(&dev->fsa_dev[cid].sense_data,
> + HARDWARE_ERROR, SENCODE_INTERNAL_TARGET_FAILURE,
> + ASENCODE_INTERNAL_TARGET_FAILURE, 0, 0);
> + memcpy(scsicmd->sense_buffer, &dev->fsa_dev[cid].sense_data,
> + min_t(size_t, sizeof(dev->fsa_dev[cid].sense_data),
> + SCSI_SENSE_BUFFERSIZE));
> + scsicmd->scsi_done(scsicmd);
> + return 1;
> + }
> +
> dprintk((KERN_DEBUG "aac_read[cpu %d]: lba = %llu, t = %ld.\n",
> smp_processor_id(), (unsigned long long)lba, jiffies));
> if (aac_adapter_bounds(dev,scsicmd,lba))
> @@ -1688,6 +1705,7 @@
> int status;
> struct aac_dev *dev;
> struct fib * cmd_fibcontext;
> + int cid;
>
> dev = (struct aac_dev *)scsicmd->device->host->hostdata;
> /*
> @@ -1727,6 +1745,22 @@
> count = (scsicmd->cmnd[7] << 8) | scsicmd->cmnd[8];
> fua = scsicmd->cmnd[1] & 0x8;
> }
> +
> + if ((lba + count) > (dev->fsa_dev[scmd_id(scsicmd)].size)) {
> + cid = scmd_id(scsicmd);
> + dprintk((KERN_DEBUG "aacraid: Illegal lba\n"));
> + scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
> + SAM_STAT_CHECK_CONDITION;
> + set_sense(&dev->fsa_dev[cid].sense_data,
> + HARDWARE_ERROR, SENCODE_INTERNAL_TARGET_FAILURE,
> + ASENCODE_INTERNAL_TARGET_FAILURE, 0, 0);
> + memcpy(scsicmd->sense_buffer, &dev->fsa_dev[cid].sense_data,
> + min_t(size_t, sizeof(dev->fsa_dev[cid].sense_data),
> + SCSI_SENSE_BUFFERSIZE));
> + scsicmd->scsi_done(scsicmd);
> + return 1;
> + }
> +
> dprintk((KERN_DEBUG "aac_write[cpu %d]: lba = %llu, t = %ld.\n",
> smp_processor_id(), (unsigned long long)lba, jiffies));
> if (aac_adapter_bounds(dev,scsicmd,lba))
For some reason, this patch is whitespace damaged ... all the tabs have
become spaces ... I fixed it up (for this one time).
James
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 5/5] AACRAID driver update
2010-05-01 19:03 ` James Bottomley
` (3 preceding siblings ...)
2010-05-10 11:24 ` [PATCH 4/5] " Rajashekhara, Mahesh
@ 2010-05-10 11:29 ` Rajashekhara, Mahesh
4 siblings, 0 replies; 9+ messages in thread
From: Rajashekhara, Mahesh @ 2010-05-10 11:29 UTC (permalink / raw)
To: James Bottomley, linux-scsi@vger.kernel.org; +Cc: AACRAID
Hi All,
Below is the patch details:
Problem description:
--------------------
The problem reported by one of the customer was when a logical array is deleted(from the SDK, from the GUI,
from arcconf) then the corresponding physical device (/dev/sdb, for example) is not removed from the Linux
namespace. So you end up with a "dead" device entry. And some of the linux tools go slightly wonky.
Solution:
---------
Based on the notification from FW, the driver calls "scsi_remove_device" for the DELETED drive. This call not
only informs the scsi device status to the SCSI mid layer and also it will remove corresponding scsi device
entries from the Linux sysfs.
Please see patch below.
=====================
Mahesh
Linux Driver Development Engineer,
Adaptec ODC, Bangalore
This attached patch is against current scsi-misc-2.6.
ObligatoryDisclaimer: Please accept my condolences regarding Outlook's handling of patch attachments (inline gets damaged, please use attachment).
Signed-off-by: Mahesh Rajashekhara <aacraid@adaptec.com>
drivers/scsi/aacraid/aacraid.h | 4 +++-
drivers/scsi/aacraid/commsup.c | 8 ++++++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
--- a/drivers/scsi/aacraid/aacraid.h 2010-05-13 13:11:54.752924936 -0700
+++ b/drivers/scsi/aacraid/aacraid.h 2010-05-13 13:15:41.923389776 -0700
@@ -12,7 +12,7 @@
*----------------------------------------------------------------------------*/
#ifndef AAC_DRIVER_BUILD
-# define AAC_DRIVER_BUILD 24702
+# define AAC_DRIVER_BUILD 26400
# define AAC_DRIVER_BRANCH "-ms"
#endif
#define MAXIMUM_NUM_CONTAINERS 32
@@ -26,6 +26,8 @@
#define AAC_MAX_HOSTPHYSMEMPAGES (0xfffff)
#define AAC_MAX_32BIT_SGBCOUNT ((unsigned short)256)
+#define AAC_DEBUG_INSTRUMENT_AIF_DELETE
+
/*
* These macros convert from physical channels to virtual channels
*/
diff -ru a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
--- a/drivers/scsi/aacraid/commsup.c 2010-05-13 13:11:54.752924936 -0700
+++ b/drivers/scsi/aacraid/commsup.c 2010-05-13 13:19:45.767319864 -0700
@@ -1123,6 +1123,9 @@
if (device) {
switch (device_config_needed) {
case DELETE:
+#if (defined(AAC_DEBUG_INSTRUMENT_AIF_DELETE))
+ scsi_remove_device(device);
+#else
if (scsi_device_online(device)) {
scsi_device_set_state(device, SDEV_OFFLINE);
sdev_printk(KERN_INFO, device,
@@ -1131,6 +1134,7 @@
"array deleted" :
"enclosure services event");
}
+#endif
break;
case ADD:
if (!scsi_device_online(device)) {
@@ -1145,12 +1149,16 @@
case CHANGE:
if ((channel == CONTAINER_CHANNEL)
&& (!dev->fsa_dev[container].valid)) {
+#if (defined(AAC_DEBUG_INSTRUMENT_AIF_DELETE))
+ scsi_remove_device(device);
+#else
if (!scsi_device_online(device))
break;
scsi_device_set_state(device, SDEV_OFFLINE);
sdev_printk(KERN_INFO, device,
"Device offlined - %s\n",
"array failed");
+#endif
break;
}
scsi_rescan_device(&device->sdev_gendev);
Regards - Mahesh Rajashekhara
-----Original Message-----
From: James Bottomley [mailto:James.Bottomley@suse.de]
Sent: Sunday, May 02, 2010 12:33 AM
To: Rajashekhara, Mahesh
Cc: linux-scsi@vger.kernel.org; AACRAID
Subject: Re: AACRAID driver update patch version - 26400
On Wed, 2010-04-07 at 20:55 -0700, Rajashekhara, Mahesh wrote:
> (Resending in plain text as the earlier email got bounced)
>
> Hi All,
>
> We have prepared driver patch from latest driver source.
>
> We worked on a issue related to SMARTCTL([Bugme-new] [Bug 15623] New: doesn't allow SMART-passthrough on Adaptec Series 5 controller)
> and found the root cause of this problem. We have responded to this bug, stating that we will be releasing the driver patch for addressing this problem.
> Also, in our recent program releases we have addressed some more issues.
>
> Please look into the release notes.
Please, no. What I actually need is a set of patches (one per change or
feature) with a properly added description and signoff as described in
Documentation/SubmittingPatches.
> Please find the attachments.
> 1. aacraid-version-26400-040510-patch - Driver patch
> 2. aacraid-scsi-misc-2.6-warnings.txt - Warning message text
> 3. Release notes
So if I look at the Changelog it seems that there should be four
separate changes:
> AACRAID Driver for Linux - version 26400
> ----------------------------------------
>
> Addressed issues:
> ----------------
> 1. The default driver setting is "expose_physicals=0", which means raw
> physical drives not exposed to OS.
> If the user wants to expose connected physical drives, enable
> "expose_physicals" module parameter.
> With the new JBOD firmware, physical drives are not available for
> "expose_physicals>0".
> In function "aac_expose_phy_device", added to reset the appropriate
> bit in the first byte of inquiry data.
> This fix exposes the connected physical drives.
OK
> 2. Added support for handling ATA pass-through commands.
> When the "CC" bit is set by the host in ATA pass-through CDB,
> driver is supposed to return DID_OK
> When the "CC" bit is reset by the host, driver should return
> DID_ERROR
No, that's not correct. You should return a result code of
SAM_STAT_CHECK_CONDITION, with a driver byte of DID_OK. If you return
DID_ERROR, it will unconditionally trigger error handling and a retry.
> 3. Firmware exposes lesser container capacity than the actual one.
> It exposes [actaul size - hidden reserved space(10MB)] to the OS,
> IO's to the 10MB should be prohibhited from the Linux
> driver.
> When the IO's falls into hidden reserved space, driver internally
> sets sensekey to HARDWARE_ERROR and sends notification to the SCSI mid
> layer.
> Added "expose_hidden_space" flag, by default the fix will be
> executed.
> Only if the user sets "expose_hidden_space=1", user can access
> beyond the array reported size(hidden reserved space 10MB).
This doesn't exactly look like the right interface. Won't the user want
the hidden space exposed on a per array basis? In which case a sysfs
flag in the host attributes would be far more appropriate than a module
flag?
> 4. Based on the notification from firmware, the driver sets up
> SDV_OFFLINE flag for a deleted array and
> the SCSI mid layer comes to know the attached SCSI device has gone
> offline. But with
> this notification, scsi device entries are not being removed.
> Enabled "AAC_DEBUG_INSTRUMENT_AIF_DELETE" flag in the header file,
> with this flag the driver uses "scsi_remove_device".
> On the notification from firmware, the driver calls
> "scsi_remove_device" for the deleted array.
> This call not only informs the scsi device status to the SCSI mid
> layer but also
> removes corresponding scsi device entry from the linux
> sysfs.
OK, don't understand this. As noted below, the define
AAC_DEBUG_INSTRUMENT_AIF_DELETE is set by your patch, but never appears
at all in the driver ... is there missing code?
> Thanks & Regards,
> Mahesh
> Linux Driver Development Engineer,
> Adaptec ODC, Bangalore
> diff -ru a/drivers/scsi/aacraid/aachba.c
> b/drivers/scsi/aacraid/aachba.c
> --- a/drivers/scsi/aacraid/aachba.c 2010-04-03 05:49:52.000000000
> -0700
> +++ b/drivers/scsi/aacraid/aachba.c 2010-04-07 12:35:36.468017040
> -0700
> @@ -109,6 +109,16 @@
> #define BYTE2(x) (unsigned char)((x) >> 16)
> #define BYTE3(x) (unsigned char)((x) >> 24)
>
> +
> +/* ATA pass thru commands */
> +#ifndef ATA_12
> +#define ATA_12 0xa1 /* 12-byte pass-thru */
> +#endif
> +
> +#ifndef ATA_16
> +#define ATA_16 0x85 /* 16-byte pass-thru */
> +#endif
> +
This chunk is clearly wrong ... those defines are in include/scsi/scsi.h
> @@ -1647,6 +1671,27 @@
> count = (scsicmd->cmnd[7] << 8) | scsicmd->cmnd[8];
> break;
> }
> +
> + if (expose_hidden_space <= 0) {
> + if ((lba + count) > (dev->fsa_dev[scmd_id(scsicmd)].size)) {
> + int cid = scmd_id(scsicmd);
> + dprintk((KERN_DEBUG "aacraid: Illegal lba\n"));
> + scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
> + SAM_STAT_CHECK_CONDITION;
> + set_sense(&dev->fsa_dev[cid].sense_data,
> + HARDWARE_ERROR,
> + SENCODE_INTERNAL_TARGET_FAILURE,
> + ASENCODE_INTERNAL_TARGET_FAILURE, 0, 0);
> + memcpy(scsicmd->sense_buffer,
> + &dev->fsa_dev[cid].sense_data,
> + min_t(size_t,
> + sizeof(dev->fsa_dev[cid].sense_data),
> + SCSI_SENSE_BUFFERSIZE));
> + scsicmd->scsi_done(scsicmd);
> + return 1;
> + }
> + }
> +
This chunk is correctly indented, that's great.
> dprintk((KERN_DEBUG "aac_read[cpu %d]: lba = %llu, t = %ld.\n",
> smp_processor_id(), (unsigned long long)lba, jiffies));
> if (aac_adapter_bounds(dev,scsicmd,lba))
> @@ -1727,6 +1772,26 @@
> count = (scsicmd->cmnd[7] << 8) | scsicmd->cmnd[8];
> fua = scsicmd->cmnd[1] & 0x8;
> }
> +
> + if (expose_hidden_space <= 0) {
> + if ((lba + count) > (dev->fsa_dev[scmd_id(scsicmd)].size)) {
> + int cid = scmd_id(scsicmd);
> + dprintk((KERN_DEBUG "aacraid: Illegal lba\n"));
> + scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |
> + SAM_STAT_CHECK_CONDITION;
> + set_sense(&dev->fsa_dev[cid].sense_data,
> + HARDWARE_ERROR, SENCODE_INTERNAL_TARGET_FAILURE,
> + ASENCODE_INTERNAL_TARGET_FAILURE, 0, 0);
> + memcpy(scsicmd->sense_buffer,
> + &dev->fsa_dev[cid].sense_data,
> + min_t(size_t,
> + sizeof(dev->fsa_dev[cid].sense_data),
> + SCSI_SENSE_BUFFERSIZE));
> +
> + scsicmd->scsi_done(scsicmd);
> + return 1;
> + }
> + }
This one, and numerous others are wrongly indented ... the continuation
lines should have an extra tab or be aligned under the opening bracket
for a function/macro. Indentation like this makes the file very hard to
read.
> diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
> --- a/drivers/scsi/aacraid/aacraid.h 2010-04-03 05:49:52.000000000 -0700
> +++ b/drivers/scsi/aacraid/aacraid.h 2010-04-07 14:17:18.409380952 -0700
> @@ -1,6 +1,7 @@
> #ifndef dprintk
> # define dprintk(x)
> #endif
> +#define AAC_DEBUG_INSTRUMENT_AIF_DELETE
What is this? It's not used anywhere else in the file (a leftover from
some removed debugging code?)
> @@ -1065,6 +1065,10 @@
> #define aac_adapter_ioremap(dev, size) \
> (dev)->a_ops.adapter_ioremap(dev, size)
>
> +#define aac_adapter_intr(dev) \
> + ((dev)->a_ops.adapter_intr((dev)->pdev->irq, \
> + (void *)dev))
> +
This is another very weird macro, but as far as I can tell, it's never
used either.
> --- a/drivers/scsi/aacraid/commctrl.c 2010-04-03 05:49:52.000000000
> -0700
> +++ b/drivers/scsi/aacraid/commctrl.c 2010-04-07 12:52:53.803318104
> -0700
> @@ -128,13 +128,13 @@
> retval =
> aac_fib_send(le16_to_cpu(kfib->header.Command), fibptr,
> le16_to_cpu(kfib->header.Size) ,
> FsaNormal,
> 1, 1, NULL, NULL);
> - if (retval) {
> - goto cleanup;
> - }
> if (aac_fib_complete(fibptr) != 0) {
> + if (!retval)
> retval = -EINVAL;
> - goto cleanup;
> + goto cleanup;
This looks wrong ... it's wrongly indented, but I'm assuming you meant
to remove the goto altogether so it gets swept up in the code below?
> }
> + if (retval)
> + goto cleanup;
> @@ -534,6 +532,13 @@
> "update mother board
> BIOS or consider utilizing one of\n"
> "the SAFE mode
> kernel options (acpi, apic etc)\n");
> }
> + spin_lock_irqsave(&fibptr->event_lock,
> + flags);
> + fibptr->done = 2;
> + spin_unlock_irqrestore(
> + &fibptr->event_lock,
> + flags);
So these locks ... even badly indented ... don't look necessary:
fibptr->done is a u32. On all architectures that's updated atomically.
This means that the check on fibptr->done always reads either the old
value or the new value ... there's no need to add extra locking unless
there are other values or states that need updating atomically.
> + dprintk((KERN_ERR "aacraid:
> sync. command timed out after 180 seconds\n"));
James
^ permalink raw reply [flat|nested] 9+ messages in thread