Re: [PATCH] Marvell 6440 SAS/SATA driver

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Sat, 2008-01-26 at 00:43 +0800, Ke Wei wrote:

>  struct mvs_phy {
>         struct mvs_port         *port;
>         struct asd_sas_phy      sas_phy;
> +       struct sas_identify identify;
> +       __le32          dev_info;
> +       __le64          dev_sas_addr;
> +       __le32          att_dev_info;
> +       __le64          att_dev_sas_addr;
> +       u32             type;
> +       __le32          phy_status;
> +       __le32          irq_status;
> +       u8              wide_port_phymap;
> +       u32             frame_rcvd_size;
> +       u8              frame_rcvd[32];
>  
> -       u8                      frame_rcvd[24 + 1024];
>  };

These __le quantites don't look right ... they're all read in by readl,
which will convert little endian to CPU anyway.


> @@ -437,27 +586,55 @@ struct mvs_info {
>         dma_addr_t              rx_dma;
>         u32                     rx_cons;        /* RX consumer idx */
>  
> -       __le32                  *rx_fis;        /* RX'd FIS area */
> +       void                    *rx_fis;        /* RX'd FIS area */

Now the received FIS, on the other hand, provided you're storing it in
wire format (which you look to be) *is* little endian data by definition
in the ATA spec.


> -static void mvs_tag_clear(struct mvs_info *mvi, unsigned int tag)
> +static void mvs_tag_clear(struct mvs_info *mvi, u32 tag)
>  {
> -       mvi->tags[tag / sizeof(unsigned long)] &=
> -               ~(1UL << (tag % sizeof(unsigned long)));
> +       mvi->tag_in = (mvi->tag_in + 1) & (MVS_SLOTS - 1);
> +       mvi->tags[mvi->tag_in] = tag;
>  }
>  
> -static void mvs_tag_set(struct mvs_info *mvi, unsigned int tag)
> +static void mvs_tag_free(struct mvs_info *mvi, u32 tag)
>  {
> -       mvi->tags[tag / sizeof(unsigned long)] |=
> -               (1UL << (tag % sizeof(unsigned long)));
> +       mvi->tag_out = (mvi->tag_out - 1) & (MVS_SLOTS - 1);
>  }
>  
> -static bool mvs_tag_test(struct mvs_info *mvi, unsigned int tag)
> +static int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out)
>  {
> -       return mvi->tags[tag / sizeof(unsigned long)] &
> -               (1UL << (tag % sizeof(unsigned long)));
> +       if (mvi->tag_out != mvi->tag_in) {
> +               *tag_out = mvi->tags[mvi->tag_out];
> +               mvi->tag_out = (mvi->tag_out + 1) & (MVS_SLOTS - 1);
> +               return 0;
> +       }
> +       return -EBUSY;

I really don't think you should be doing this.  That single ring governs
all the potential tag slots for everything in this device.  If you do a
simple head tail allocation, what can happen is that you get a slow tag
(attached to a format command, or a tape command) and then the ring head
will hit the slow tag and the entire device will halt.  I think you need
a bitmap based allocation algorithm to ensure that if you have a free
tag anywhere, you'll use it.

If you look at the aic94xx index functions in aic94xx_hwi.h you'll see
asd_tc_index_get() and asd_tc_index_release() doing exactly what you
want with the native linux bitmap functions (the aic also uses a single
issue queue with indexes into it).

James