Re: ide-tape redux (was: Re:)

2008-02-09 Thread Bartlomiej Zolnierkiewicz
On Wednesday 06 February 2008, Borislav Petkov wrote:
> ... and while we're at it ...
> 
> commit c824f79fe4040f7541d7e35c546bb57a22d2fe11
> Author: Borislav Petkov <[EMAIL PROTECTED]>
> Date:   Wed Feb 6 06:23:10 2008 +0100
> 
> ide-tape: move all struct and other defs to the top
> 
> Signed-off-by: Borislav Petkov <[EMAIL PROTECTED]>

looks good, but please do it before
"remove atomic test/set macros" patch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: ide-tape redux (was: Re:)

2008-02-09 Thread Bartlomiej Zolnierkiewicz

Hi,

On Wednesday 06 February 2008, Borislav Petkov wrote:
> On Tue, Feb 05, 2008 at 02:20:22AM +0100, Bartlomiej Zolnierkiewicz wrote:
> 
> [...]
> 
> > w.r.t. #11 ide-tape uses char devices and supports DSC so it is not as 
> > obvious
> > as in ide-floppy case that all atomic bitops can be just removed (extra 
> > audit
> > and some time -mm are required) so please resync/resubmit
> 
> Ok, here's what i think we should do here: There are two flags that handle 
> DSC:
> PC_FL_WAIT_FOR_DSC and IDETAPE_FL_IGNORE_DSC. The first one is per pc and is 
> set in
> all the packet command init functions ..create_bla_cmd() after their callers 
> have
> created a pc on the stack and reached its ptr down for initialization. This 
> case
> is carefree since the bit will be tested first in the interrupt handler and 
> this
> happens only after the pc is queued (ide_do_drive_cmd()) into the request 
> buffer.
> 
> The other flag, IDETAPE_FL_IGNORE_DSC, is polled for in the request handler 
> and
> can be set when a pc is being retried and we should leave only those atomic
> tests intact, imho, but i'm definitely gonna need a second opinion here.

Actually for this flag it looks safe because the new request shouldn't be
queued while the old one is still active (IOW there should be no simultaneous
idetape_do_request() and idetape_end_request() calls).  OTOH some other
flags (especially IDETAPE_PIPELINE_ACTIVE) looks un-safe (pileline can be
active while ->open on character device happens).

Given that this driver is currently scheduled for removal and that complete
audit/analysis would be quite costly I don't think that removing atomic bitops
from tape->flags is worth it (for pc->flags it is fine).

Could you please recast the patch accordingly?

[ + some minor issues below ]

> ---
> 
> commit 1ed8ae92249d5dff7af4ee88710ea08ff3f3356f
> Author: Borislav Petkov <[EMAIL PROTECTED]>
> Date:   Tue Feb 5 08:05:35 2008 +0100
> 
> ide-tape: remove atomic test/set macros
> 
> Also, since the driver supports DSC, leave the atomic tests
> for the IDETAPE_FL_IGNORE_DSC bit untouched because this is polled
> for in the request handler and can be set in the interrupt
> handler through idetape_retry_pc() after enabling interrupts.
> 
> Finally, remove flag IDETAPE_READ_ERROR since it is unused.
> 
> Signed-off-by: Borislav Petkov <[EMAIL PROTECTED]>
> 
> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
> index e59e49e..9455ce4 100644
> --- a/drivers/ide/ide-tape.c
> +++ b/drivers/ide/ide-tape.c
> @@ -206,24 +206,24 @@ typedef struct idetape_packet_command_s {
>   /* Temporary buffer */
>   u8 pc_buffer[IDETAPE_PC_BUFFER_SIZE];
>   /* Status/Action bit flags: long for set_bit */
> - unsigned long flags;
> + unsigned int flags;

please leave it as 'unsigned long' to match other drivers

>  } idetape_pc_t;
>  
> -/*
> - *   Packet command flag bits.
> - */
> -/* Set when an error is considered normal - We won't retry */
> -#define  PC_ABORT0
> -/* 1 When polling for DSC on a media access command */
> -#define PC_WAIT_FOR_DSC  1
> -/* 1 when we prefer to use DMA if possible */
> -#define PC_DMA_RECOMMENDED   2
> -/* 1 while DMA in progress */
> -#define  PC_DMA_IN_PROGRESS  3
> -/* 1 when encountered problem during DMA */
> -#define  PC_DMA_ERROR4
> -/* Data direction */
> -#define  PC_WRITING  5
> +/* Packet command flag bits. */
> +enum {
> + /* Set when an error is considered normal - We won't retry */
> + PC_FL_ABORT = (1 << 0),
> + /* 1 When polling for DSC on a media access command */
> + PC_FL_WAIT_FOR_DSC  = (1 << 1),
> + /* 1 when we prefer to use DMA if possible */
> + PC_FL_DMA_RECOMMENDED   = (1 << 2),
> + /* 1 while DMA in progress */
> + PC_FL_DMA_IN_PROGRESS   = (1 << 3),
> + /* 1 when encountered problem during DMA */
> + PC_FL_DMA_ERROR = (1 << 4),
> + /* Data direction */
> + PC_FL_WRITING   = (1 << 5),

Why not PC_FLAG_*?  [ It would match flags in ide-floppy ]

> +};
>  
>  /* A pipeline stage. */
>  typedef struct idetape_stage_s {
> @@ -357,8 +357,7 @@ typedef struct ide_tape_obj {
>   /* Wasted space in each stage */
>   int excess_bh_size;
>  
> - /* Status/Action flags: long for set_bit */
> - unsigned long flags;
> + unsigned int flags;

please leave it as 'unsigned long'

[ besides I don't think that mixing atomic and non-atomic access on
  the _same_ variable is OK ]

>   /* protects the ide-tape queue */
>   spinlock_t lock;
>  
> @@ -451,20 +450,26 @@ static void ide_tape_put(struct ide_tape_obj *tape)
>  #define DOOR_LOCKED  1
>  #define DOOR_EXPLICITLY_LOCKED   2
>  
> -/*
> - *   Tape flag bits values.
> - */
> -#define IDETAPE_IGNORE_DSC   0
> -#define IDETAPE_ADDRESS_VALID 

Re: ide-tape redux (was: Re:)

2008-02-09 Thread Bartlomiej Zolnierkiewicz
On Wednesday 06 February 2008, Borislav Petkov wrote:
 ... and while we're at it ...
 
 commit c824f79fe4040f7541d7e35c546bb57a22d2fe11
 Author: Borislav Petkov [EMAIL PROTECTED]
 Date:   Wed Feb 6 06:23:10 2008 +0100
 
 ide-tape: move all struct and other defs to the top
 
 Signed-off-by: Borislav Petkov [EMAIL PROTECTED]

looks good, but please do it before
remove atomic test/set macros patch
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: ide-tape redux (was: Re:)

2008-02-09 Thread Bartlomiej Zolnierkiewicz

Hi,

On Wednesday 06 February 2008, Borislav Petkov wrote:
 On Tue, Feb 05, 2008 at 02:20:22AM +0100, Bartlomiej Zolnierkiewicz wrote:
 
 [...]
 
  w.r.t. #11 ide-tape uses char devices and supports DSC so it is not as 
  obvious
  as in ide-floppy case that all atomic bitops can be just removed (extra 
  audit
  and some time -mm are required) so please resync/resubmit
 
 Ok, here's what i think we should do here: There are two flags that handle 
 DSC:
 PC_FL_WAIT_FOR_DSC and IDETAPE_FL_IGNORE_DSC. The first one is per pc and is 
 set in
 all the packet command init functions ..create_bla_cmd() after their callers 
 have
 created a pc on the stack and reached its ptr down for initialization. This 
 case
 is carefree since the bit will be tested first in the interrupt handler and 
 this
 happens only after the pc is queued (ide_do_drive_cmd()) into the request 
 buffer.
 
 The other flag, IDETAPE_FL_IGNORE_DSC, is polled for in the request handler 
 and
 can be set when a pc is being retried and we should leave only those atomic
 tests intact, imho, but i'm definitely gonna need a second opinion here.

Actually for this flag it looks safe because the new request shouldn't be
queued while the old one is still active (IOW there should be no simultaneous
idetape_do_request() and idetape_end_request() calls).  OTOH some other
flags (especially IDETAPE_PIPELINE_ACTIVE) looks un-safe (pileline can be
active while -open on character device happens).

Given that this driver is currently scheduled for removal and that complete
audit/analysis would be quite costly I don't think that removing atomic bitops
from tape-flags is worth it (for pc-flags it is fine).

Could you please recast the patch accordingly?

[ + some minor issues below ]

 ---
 
 commit 1ed8ae92249d5dff7af4ee88710ea08ff3f3356f
 Author: Borislav Petkov [EMAIL PROTECTED]
 Date:   Tue Feb 5 08:05:35 2008 +0100
 
 ide-tape: remove atomic test/set macros
 
 Also, since the driver supports DSC, leave the atomic tests
 for the IDETAPE_FL_IGNORE_DSC bit untouched because this is polled
 for in the request handler and can be set in the interrupt
 handler through idetape_retry_pc() after enabling interrupts.
 
 Finally, remove flag IDETAPE_READ_ERROR since it is unused.
 
 Signed-off-by: Borislav Petkov [EMAIL PROTECTED]
 
 diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
 index e59e49e..9455ce4 100644
 --- a/drivers/ide/ide-tape.c
 +++ b/drivers/ide/ide-tape.c
 @@ -206,24 +206,24 @@ typedef struct idetape_packet_command_s {
   /* Temporary buffer */
   u8 pc_buffer[IDETAPE_PC_BUFFER_SIZE];
   /* Status/Action bit flags: long for set_bit */
 - unsigned long flags;
 + unsigned int flags;

please leave it as 'unsigned long' to match other drivers

  } idetape_pc_t;
  
 -/*
 - *   Packet command flag bits.
 - */
 -/* Set when an error is considered normal - We won't retry */
 -#define  PC_ABORT0
 -/* 1 When polling for DSC on a media access command */
 -#define PC_WAIT_FOR_DSC  1
 -/* 1 when we prefer to use DMA if possible */
 -#define PC_DMA_RECOMMENDED   2
 -/* 1 while DMA in progress */
 -#define  PC_DMA_IN_PROGRESS  3
 -/* 1 when encountered problem during DMA */
 -#define  PC_DMA_ERROR4
 -/* Data direction */
 -#define  PC_WRITING  5
 +/* Packet command flag bits. */
 +enum {
 + /* Set when an error is considered normal - We won't retry */
 + PC_FL_ABORT = (1  0),
 + /* 1 When polling for DSC on a media access command */
 + PC_FL_WAIT_FOR_DSC  = (1  1),
 + /* 1 when we prefer to use DMA if possible */
 + PC_FL_DMA_RECOMMENDED   = (1  2),
 + /* 1 while DMA in progress */
 + PC_FL_DMA_IN_PROGRESS   = (1  3),
 + /* 1 when encountered problem during DMA */
 + PC_FL_DMA_ERROR = (1  4),
 + /* Data direction */
 + PC_FL_WRITING   = (1  5),

Why not PC_FLAG_*?  [ It would match flags in ide-floppy ]

 +};
  
  /* A pipeline stage. */
  typedef struct idetape_stage_s {
 @@ -357,8 +357,7 @@ typedef struct ide_tape_obj {
   /* Wasted space in each stage */
   int excess_bh_size;
  
 - /* Status/Action flags: long for set_bit */
 - unsigned long flags;
 + unsigned int flags;

please leave it as 'unsigned long'

[ besides I don't think that mixing atomic and non-atomic access on
  the _same_ variable is OK ]

   /* protects the ide-tape queue */
   spinlock_t lock;
  
 @@ -451,20 +450,26 @@ static void ide_tape_put(struct ide_tape_obj *tape)
  #define DOOR_LOCKED  1
  #define DOOR_EXPLICITLY_LOCKED   2
  
 -/*
 - *   Tape flag bits values.
 - */
 -#define IDETAPE_IGNORE_DSC   0
 -#define IDETAPE_ADDRESS_VALID1   /* 0 When the tape 
 position is unknown */
 -#define IDETAPE_BUSY 2   /* Device already 

Re: ide-tape redux (was: Re:)

2008-02-05 Thread Borislav Petkov
... and while we're at it ...

commit c824f79fe4040f7541d7e35c546bb57a22d2fe11
Author: Borislav Petkov <[EMAIL PROTECTED]>
Date:   Wed Feb 6 06:23:10 2008 +0100

ide-tape: move all struct and other defs to the top

Signed-off-by: Borislav Petkov <[EMAIL PROTECTED]>

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 9455ce4..398aea8 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -225,6 +225,69 @@ enum {
PC_FL_WRITING   = (1 << 5),
 };
 
+/* Tape door status */
+#define DOOR_UNLOCKED  0
+#define DOOR_LOCKED1
+#define DOOR_EXPLICITLY_LOCKED 2
+
+/* Tape flag bits values. */
+enum {
+   IDETAPE_FL_IGNORE_DSC   = (1 << 0),
+   /* 0 When the tape position is unknown */
+   IDETAPE_FL_ADDRESS_VALID= (1 << 1),
+   /* Device already opened */
+   IDETAPE_FL_BUSY = (1 << 2),
+   /* Error detected in a pipeline stage */
+   IDETAPE_FL_PIPELINE_ERR = (1 << 3),
+   /* Attempt to auto-detect the current user block size */
+   IDETAPE_FL_DETECT_BS= (1 << 4),
+   /* Currently on a filemark */
+   IDETAPE_FL_FILEMARK = (1 << 5),
+   /* DRQ interrupt device */
+   IDETAPE_FL_DRQ_INTERRUPT= (1 << 6),
+   /* pipeline active */
+   IDETAPE_FL_PIPELINE_ACTIVE  = (1 << 7),
+   /* 0 = no tape is loaded, so we don't rewind after ejecting */
+   IDETAPE_FL_MEDIUM_PRESENT   = (1 << 8),
+};
+
+/* A define for the READ BUFFER command */
+#define IDETAPE_RETRIEVE_FAULTY_BLOCK  6
+
+/* Some defines for the SPACE command */
+#define IDETAPE_SPACE_OVER_FILEMARK1
+#define IDETAPE_SPACE_TO_EOD   3
+
+/* Some defines for the LOAD UNLOAD command */
+#define IDETAPE_LU_LOAD_MASK   1
+#define IDETAPE_LU_RETENSION_MASK  2
+#define IDETAPE_LU_EOT_MASK4
+
+/*
+ * Special requests for our block device strategy routine.
+ *
+ * In order to service a character device command, we add special requests to
+ * the tail of our block device request queue and wait for their completion.
+ */
+
+enum {
+   REQ_IDETAPE_PC1 = (1 << 0), /* packet command (first stage) */
+   REQ_IDETAPE_PC2 = (1 << 1), /* packet command (second stage) */
+   REQ_IDETAPE_READ= (1 << 2),
+   REQ_IDETAPE_WRITE   = (1 << 3),
+   REQ_IDETAPE_READ_BUFFER = (1 << 4),
+};
+
+/* Error codes returned in rq->errors to the higher part of the driver. */
+#defineIDETAPE_ERROR_GENERAL   101
+#defineIDETAPE_ERROR_FILEMARK  102
+#defineIDETAPE_ERROR_EOD   103
+
+/* Structures related to the SELECT SENSE / MODE SENSE packet commands. */
+#define IDETAPE_BLOCK_DESCRIPTOR   0
+#defineIDETAPE_CAPABILITIES_PAGE   0x2a
+
+
 /* A pipeline stage. */
 typedef struct idetape_stage_s {
struct request rq;  /* The corresponding request */
@@ -445,68 +508,6 @@ static void ide_tape_put(struct ide_tape_obj *tape)
mutex_unlock(_ref_mutex);
 }
 
-/* Tape door status */
-#define DOOR_UNLOCKED  0
-#define DOOR_LOCKED1
-#define DOOR_EXPLICITLY_LOCKED 2
-
-/* Tape flag bits values. */
-enum {
-   IDETAPE_FL_IGNORE_DSC   = (1 << 0),
-   /* 0 When the tape position is unknown */
-   IDETAPE_FL_ADDRESS_VALID= (1 << 1),
-   /* Device already opened */
-   IDETAPE_FL_BUSY = (1 << 2),
-   /* Error detected in a pipeline stage */
-   IDETAPE_FL_PIPELINE_ERR = (1 << 3),
-   /* Attempt to auto-detect the current user block size */
-   IDETAPE_FL_DETECT_BS= (1 << 4),
-   /* Currently on a filemark */
-   IDETAPE_FL_FILEMARK = (1 << 5),
-   /* DRQ interrupt device */
-   IDETAPE_FL_DRQ_INTERRUPT= (1 << 6),
-   /* pipeline active */
-   IDETAPE_FL_PIPELINE_ACTIVE  = (1 << 7),
-   /* 0 = no tape is loaded, so we don't rewind after ejecting */
-   IDETAPE_FL_MEDIUM_PRESENT   = (1 << 8),
-};
-
-/* A define for the READ BUFFER command */
-#define IDETAPE_RETRIEVE_FAULTY_BLOCK  6
-
-/* Some defines for the SPACE command */
-#define IDETAPE_SPACE_OVER_FILEMARK1
-#define IDETAPE_SPACE_TO_EOD   3
-
-/* Some defines for the LOAD UNLOAD command */
-#define IDETAPE_LU_LOAD_MASK   1
-#define IDETAPE_LU_RETENSION_MASK  2
-#define IDETAPE_LU_EOT_MASK4
-
-/*
- * Special requests for our block device strategy routine.
- *
- * In order to service a character device command, we add special requests to
- * the tail of our block device request queue and wait for their completion.
- */
-
-enum {
-   REQ_IDETAPE_PC1 = (1 << 0), /* packet command (first stage) */
-   REQ_IDETAPE_PC2 = (1 << 1), /* packet command (second stage) */
-   REQ_IDETAPE_READ= (1 << 2),
-   

Re: ide-tape redux (was: Re:)

2008-02-05 Thread Borislav Petkov
On Tue, Feb 05, 2008 at 02:20:22AM +0100, Bartlomiej Zolnierkiewicz wrote:

[...]

> w.r.t. #11 ide-tape uses char devices and supports DSC so it is not as obvious
> as in ide-floppy case that all atomic bitops can be just removed (extra audit
> and some time -mm are required) so please resync/resubmit

Ok, here's what i think we should do here: There are two flags that handle DSC:
PC_FL_WAIT_FOR_DSC and IDETAPE_FL_IGNORE_DSC. The first one is per pc and is 
set in
all the packet command init functions ..create_bla_cmd() after their callers 
have
created a pc on the stack and reached its ptr down for initialization. This case
is carefree since the bit will be tested first in the interrupt handler and this
happens only after the pc is queued (ide_do_drive_cmd()) into the request 
buffer.

The other flag, IDETAPE_FL_IGNORE_DSC, is polled for in the request handler and
can be set when a pc is being retried and we should leave only those atomic
tests intact, imho, but i'm definitely gonna need a second opinion here.

---

commit 1ed8ae92249d5dff7af4ee88710ea08ff3f3356f
Author: Borislav Petkov <[EMAIL PROTECTED]>
Date:   Tue Feb 5 08:05:35 2008 +0100

ide-tape: remove atomic test/set macros

Also, since the driver supports DSC, leave the atomic tests
for the IDETAPE_FL_IGNORE_DSC bit untouched because this is polled
for in the request handler and can be set in the interrupt
handler through idetape_retry_pc() after enabling interrupts.

Finally, remove flag IDETAPE_READ_ERROR since it is unused.

Signed-off-by: Borislav Petkov <[EMAIL PROTECTED]>

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index e59e49e..9455ce4 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -206,24 +206,24 @@ typedef struct idetape_packet_command_s {
/* Temporary buffer */
u8 pc_buffer[IDETAPE_PC_BUFFER_SIZE];
/* Status/Action bit flags: long for set_bit */
-   unsigned long flags;
+   unsigned int flags;
 } idetape_pc_t;
 
-/*
- * Packet command flag bits.
- */
-/* Set when an error is considered normal - We won't retry */
-#definePC_ABORT0
-/* 1 When polling for DSC on a media access command */
-#define PC_WAIT_FOR_DSC1
-/* 1 when we prefer to use DMA if possible */
-#define PC_DMA_RECOMMENDED 2
-/* 1 while DMA in progress */
-#definePC_DMA_IN_PROGRESS  3
-/* 1 when encountered problem during DMA */
-#definePC_DMA_ERROR4
-/* Data direction */
-#definePC_WRITING  5
+/* Packet command flag bits. */
+enum {
+   /* Set when an error is considered normal - We won't retry */
+   PC_FL_ABORT = (1 << 0),
+   /* 1 When polling for DSC on a media access command */
+   PC_FL_WAIT_FOR_DSC  = (1 << 1),
+   /* 1 when we prefer to use DMA if possible */
+   PC_FL_DMA_RECOMMENDED   = (1 << 2),
+   /* 1 while DMA in progress */
+   PC_FL_DMA_IN_PROGRESS   = (1 << 3),
+   /* 1 when encountered problem during DMA */
+   PC_FL_DMA_ERROR = (1 << 4),
+   /* Data direction */
+   PC_FL_WRITING   = (1 << 5),
+};
 
 /* A pipeline stage. */
 typedef struct idetape_stage_s {
@@ -357,8 +357,7 @@ typedef struct ide_tape_obj {
/* Wasted space in each stage */
int excess_bh_size;
 
-   /* Status/Action flags: long for set_bit */
-   unsigned long flags;
+   unsigned int flags;
/* protects the ide-tape queue */
spinlock_t lock;
 
@@ -451,20 +450,26 @@ static void ide_tape_put(struct ide_tape_obj *tape)
 #define DOOR_LOCKED1
 #define DOOR_EXPLICITLY_LOCKED 2
 
-/*
- * Tape flag bits values.
- */
-#define IDETAPE_IGNORE_DSC 0
-#define IDETAPE_ADDRESS_VALID  1   /* 0 When the tape position is 
unknown */
-#define IDETAPE_BUSY   2   /* Device already opened */
-#define IDETAPE_PIPELINE_ERROR 3   /* Error detected in a pipeline 
stage */
-#define IDETAPE_DETECT_BS  4   /* Attempt to auto-detect the 
current user block size */
-#define IDETAPE_FILEMARK   5   /* Currently on a filemark */
-#define IDETAPE_DRQ_INTERRUPT  6   /* DRQ interrupt device */
-#define IDETAPE_READ_ERROR 7
-#define IDETAPE_PIPELINE_ACTIVE8   /* pipeline active */
-/* 0 = no tape is loaded, so we don't rewind after ejecting */
-#define IDETAPE_MEDIUM_PRESENT 9
+/* Tape flag bits values. */
+enum {
+   IDETAPE_FL_IGNORE_DSC   = (1 << 0),
+   /* 0 When the tape position is unknown */
+   IDETAPE_FL_ADDRESS_VALID= (1 << 1),
+   /* Device already opened */
+   IDETAPE_FL_BUSY = (1 << 2),
+   /* Error detected in a pipeline stage */
+   IDETAPE_FL_PIPELINE_ERR = (1 << 3),
+   /* Attempt to auto-detect the 

Re: ide-tape redux (was: Re:)

2008-02-05 Thread Borislav Petkov
On Tue, Feb 05, 2008 at 02:20:22AM +0100, Bartlomiej Zolnierkiewicz wrote:

[...]

 w.r.t. #11 ide-tape uses char devices and supports DSC so it is not as obvious
 as in ide-floppy case that all atomic bitops can be just removed (extra audit
 and some time -mm are required) so please resync/resubmit

Ok, here's what i think we should do here: There are two flags that handle DSC:
PC_FL_WAIT_FOR_DSC and IDETAPE_FL_IGNORE_DSC. The first one is per pc and is 
set in
all the packet command init functions ..create_bla_cmd() after their callers 
have
created a pc on the stack and reached its ptr down for initialization. This case
is carefree since the bit will be tested first in the interrupt handler and this
happens only after the pc is queued (ide_do_drive_cmd()) into the request 
buffer.

The other flag, IDETAPE_FL_IGNORE_DSC, is polled for in the request handler and
can be set when a pc is being retried and we should leave only those atomic
tests intact, imho, but i'm definitely gonna need a second opinion here.

---

commit 1ed8ae92249d5dff7af4ee88710ea08ff3f3356f
Author: Borislav Petkov [EMAIL PROTECTED]
Date:   Tue Feb 5 08:05:35 2008 +0100

ide-tape: remove atomic test/set macros

Also, since the driver supports DSC, leave the atomic tests
for the IDETAPE_FL_IGNORE_DSC bit untouched because this is polled
for in the request handler and can be set in the interrupt
handler through idetape_retry_pc() after enabling interrupts.

Finally, remove flag IDETAPE_READ_ERROR since it is unused.

Signed-off-by: Borislav Petkov [EMAIL PROTECTED]

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index e59e49e..9455ce4 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -206,24 +206,24 @@ typedef struct idetape_packet_command_s {
/* Temporary buffer */
u8 pc_buffer[IDETAPE_PC_BUFFER_SIZE];
/* Status/Action bit flags: long for set_bit */
-   unsigned long flags;
+   unsigned int flags;
 } idetape_pc_t;
 
-/*
- * Packet command flag bits.
- */
-/* Set when an error is considered normal - We won't retry */
-#definePC_ABORT0
-/* 1 When polling for DSC on a media access command */
-#define PC_WAIT_FOR_DSC1
-/* 1 when we prefer to use DMA if possible */
-#define PC_DMA_RECOMMENDED 2
-/* 1 while DMA in progress */
-#definePC_DMA_IN_PROGRESS  3
-/* 1 when encountered problem during DMA */
-#definePC_DMA_ERROR4
-/* Data direction */
-#definePC_WRITING  5
+/* Packet command flag bits. */
+enum {
+   /* Set when an error is considered normal - We won't retry */
+   PC_FL_ABORT = (1  0),
+   /* 1 When polling for DSC on a media access command */
+   PC_FL_WAIT_FOR_DSC  = (1  1),
+   /* 1 when we prefer to use DMA if possible */
+   PC_FL_DMA_RECOMMENDED   = (1  2),
+   /* 1 while DMA in progress */
+   PC_FL_DMA_IN_PROGRESS   = (1  3),
+   /* 1 when encountered problem during DMA */
+   PC_FL_DMA_ERROR = (1  4),
+   /* Data direction */
+   PC_FL_WRITING   = (1  5),
+};
 
 /* A pipeline stage. */
 typedef struct idetape_stage_s {
@@ -357,8 +357,7 @@ typedef struct ide_tape_obj {
/* Wasted space in each stage */
int excess_bh_size;
 
-   /* Status/Action flags: long for set_bit */
-   unsigned long flags;
+   unsigned int flags;
/* protects the ide-tape queue */
spinlock_t lock;
 
@@ -451,20 +450,26 @@ static void ide_tape_put(struct ide_tape_obj *tape)
 #define DOOR_LOCKED1
 #define DOOR_EXPLICITLY_LOCKED 2
 
-/*
- * Tape flag bits values.
- */
-#define IDETAPE_IGNORE_DSC 0
-#define IDETAPE_ADDRESS_VALID  1   /* 0 When the tape position is 
unknown */
-#define IDETAPE_BUSY   2   /* Device already opened */
-#define IDETAPE_PIPELINE_ERROR 3   /* Error detected in a pipeline 
stage */
-#define IDETAPE_DETECT_BS  4   /* Attempt to auto-detect the 
current user block size */
-#define IDETAPE_FILEMARK   5   /* Currently on a filemark */
-#define IDETAPE_DRQ_INTERRUPT  6   /* DRQ interrupt device */
-#define IDETAPE_READ_ERROR 7
-#define IDETAPE_PIPELINE_ACTIVE8   /* pipeline active */
-/* 0 = no tape is loaded, so we don't rewind after ejecting */
-#define IDETAPE_MEDIUM_PRESENT 9
+/* Tape flag bits values. */
+enum {
+   IDETAPE_FL_IGNORE_DSC   = (1  0),
+   /* 0 When the tape position is unknown */
+   IDETAPE_FL_ADDRESS_VALID= (1  1),
+   /* Device already opened */
+   IDETAPE_FL_BUSY = (1  2),
+   /* Error detected in a pipeline stage */
+   IDETAPE_FL_PIPELINE_ERR = (1  3),
+   /* Attempt to auto-detect the current user block size */

Re: ide-tape redux (was: Re:)

2008-02-05 Thread Borislav Petkov
... and while we're at it ...

commit c824f79fe4040f7541d7e35c546bb57a22d2fe11
Author: Borislav Petkov [EMAIL PROTECTED]
Date:   Wed Feb 6 06:23:10 2008 +0100

ide-tape: move all struct and other defs to the top

Signed-off-by: Borislav Petkov [EMAIL PROTECTED]

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 9455ce4..398aea8 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -225,6 +225,69 @@ enum {
PC_FL_WRITING   = (1  5),
 };
 
+/* Tape door status */
+#define DOOR_UNLOCKED  0
+#define DOOR_LOCKED1
+#define DOOR_EXPLICITLY_LOCKED 2
+
+/* Tape flag bits values. */
+enum {
+   IDETAPE_FL_IGNORE_DSC   = (1  0),
+   /* 0 When the tape position is unknown */
+   IDETAPE_FL_ADDRESS_VALID= (1  1),
+   /* Device already opened */
+   IDETAPE_FL_BUSY = (1  2),
+   /* Error detected in a pipeline stage */
+   IDETAPE_FL_PIPELINE_ERR = (1  3),
+   /* Attempt to auto-detect the current user block size */
+   IDETAPE_FL_DETECT_BS= (1  4),
+   /* Currently on a filemark */
+   IDETAPE_FL_FILEMARK = (1  5),
+   /* DRQ interrupt device */
+   IDETAPE_FL_DRQ_INTERRUPT= (1  6),
+   /* pipeline active */
+   IDETAPE_FL_PIPELINE_ACTIVE  = (1  7),
+   /* 0 = no tape is loaded, so we don't rewind after ejecting */
+   IDETAPE_FL_MEDIUM_PRESENT   = (1  8),
+};
+
+/* A define for the READ BUFFER command */
+#define IDETAPE_RETRIEVE_FAULTY_BLOCK  6
+
+/* Some defines for the SPACE command */
+#define IDETAPE_SPACE_OVER_FILEMARK1
+#define IDETAPE_SPACE_TO_EOD   3
+
+/* Some defines for the LOAD UNLOAD command */
+#define IDETAPE_LU_LOAD_MASK   1
+#define IDETAPE_LU_RETENSION_MASK  2
+#define IDETAPE_LU_EOT_MASK4
+
+/*
+ * Special requests for our block device strategy routine.
+ *
+ * In order to service a character device command, we add special requests to
+ * the tail of our block device request queue and wait for their completion.
+ */
+
+enum {
+   REQ_IDETAPE_PC1 = (1  0), /* packet command (first stage) */
+   REQ_IDETAPE_PC2 = (1  1), /* packet command (second stage) */
+   REQ_IDETAPE_READ= (1  2),
+   REQ_IDETAPE_WRITE   = (1  3),
+   REQ_IDETAPE_READ_BUFFER = (1  4),
+};
+
+/* Error codes returned in rq-errors to the higher part of the driver. */
+#defineIDETAPE_ERROR_GENERAL   101
+#defineIDETAPE_ERROR_FILEMARK  102
+#defineIDETAPE_ERROR_EOD   103
+
+/* Structures related to the SELECT SENSE / MODE SENSE packet commands. */
+#define IDETAPE_BLOCK_DESCRIPTOR   0
+#defineIDETAPE_CAPABILITIES_PAGE   0x2a
+
+
 /* A pipeline stage. */
 typedef struct idetape_stage_s {
struct request rq;  /* The corresponding request */
@@ -445,68 +508,6 @@ static void ide_tape_put(struct ide_tape_obj *tape)
mutex_unlock(idetape_ref_mutex);
 }
 
-/* Tape door status */
-#define DOOR_UNLOCKED  0
-#define DOOR_LOCKED1
-#define DOOR_EXPLICITLY_LOCKED 2
-
-/* Tape flag bits values. */
-enum {
-   IDETAPE_FL_IGNORE_DSC   = (1  0),
-   /* 0 When the tape position is unknown */
-   IDETAPE_FL_ADDRESS_VALID= (1  1),
-   /* Device already opened */
-   IDETAPE_FL_BUSY = (1  2),
-   /* Error detected in a pipeline stage */
-   IDETAPE_FL_PIPELINE_ERR = (1  3),
-   /* Attempt to auto-detect the current user block size */
-   IDETAPE_FL_DETECT_BS= (1  4),
-   /* Currently on a filemark */
-   IDETAPE_FL_FILEMARK = (1  5),
-   /* DRQ interrupt device */
-   IDETAPE_FL_DRQ_INTERRUPT= (1  6),
-   /* pipeline active */
-   IDETAPE_FL_PIPELINE_ACTIVE  = (1  7),
-   /* 0 = no tape is loaded, so we don't rewind after ejecting */
-   IDETAPE_FL_MEDIUM_PRESENT   = (1  8),
-};
-
-/* A define for the READ BUFFER command */
-#define IDETAPE_RETRIEVE_FAULTY_BLOCK  6
-
-/* Some defines for the SPACE command */
-#define IDETAPE_SPACE_OVER_FILEMARK1
-#define IDETAPE_SPACE_TO_EOD   3
-
-/* Some defines for the LOAD UNLOAD command */
-#define IDETAPE_LU_LOAD_MASK   1
-#define IDETAPE_LU_RETENSION_MASK  2
-#define IDETAPE_LU_EOT_MASK4
-
-/*
- * Special requests for our block device strategy routine.
- *
- * In order to service a character device command, we add special requests to
- * the tail of our block device request queue and wait for their completion.
- */
-
-enum {
-   REQ_IDETAPE_PC1 = (1  0), /* packet command (first stage) */
-   REQ_IDETAPE_PC2 = (1  1), /* packet command (second stage) */
-   REQ_IDETAPE_READ= (1  2),
-   REQ_IDETAPE_WRITE   = (1  3),
-   

ide-tape redux (was: Re:)

2008-02-04 Thread Bartlomiej Zolnierkiewicz

Hi Borislav,

On Monday 04 February 2008, Borislav Petkov wrote:
> Hi Bart,
> 
> here are the pending ide-tape patches reworked which incorporate all review
> points raised so far. Several new patches are appended to the original series
> which i thought would be reasonable to sumbit along with the others. Also,
> i've applied "ide-tape: dump gcw fields on error in idetape_identify_device()"
> which is #12 and which you can simply ignore. Furthermore, #32 from the 
> original
> series got split up into the different logical changes it dealt with, as you
> requested.

Thanks!  [ Reviewing was so much easier. ]

>  Documentation/feature-removal-schedule.txt |   14 +-
>  drivers/ide/ide-tape.c | 2764 
> +---
>  2 files changed, 1325 insertions(+), 1453 deletions(-)

applied #1-7, #9-10, #13-22 (+queued all of them for 2.6.25)

w.r.t. #8 I'm waiting for Jens to comment on blk_{get,put}_request() approach

w.r.t. #11 ide-tape uses char devices and supports DSC so it is not as obvious
as in ide-floppy case that all atomic bitops can be just removed (extra audit
and some time -mm are required) so please resync/resubmit

#12 is already in Linus' tree

Bart
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


ide-tape redux (was: Re:)

2008-02-04 Thread Bartlomiej Zolnierkiewicz

Hi Borislav,

On Monday 04 February 2008, Borislav Petkov wrote:
 Hi Bart,
 
 here are the pending ide-tape patches reworked which incorporate all review
 points raised so far. Several new patches are appended to the original series
 which i thought would be reasonable to sumbit along with the others. Also,
 i've applied ide-tape: dump gcw fields on error in idetape_identify_device()
 which is #12 and which you can simply ignore. Furthermore, #32 from the 
 original
 series got split up into the different logical changes it dealt with, as you
 requested.

Thanks!  [ Reviewing was so much easier. ]

  Documentation/feature-removal-schedule.txt |   14 +-
  drivers/ide/ide-tape.c | 2764 
 +---
  2 files changed, 1325 insertions(+), 1453 deletions(-)

applied #1-7, #9-10, #13-22 (+queued all of them for 2.6.25)

w.r.t. #8 I'm waiting for Jens to comment on blk_{get,put}_request() approach

w.r.t. #11 ide-tape uses char devices and supports DSC so it is not as obvious
as in ide-floppy case that all atomic bitops can be just removed (extra audit
and some time -mm are required) so please resync/resubmit

#12 is already in Linus' tree

Bart
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/