linux-kernel - Re: [PATCH 1/1] block: IBM RamSan 70/80 driver fixes.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <512389CB.1000906@linux.vnet.ibm.com>
Date:	Tue, 19 Feb 2013 08:18:51 -0600
From:	Brian King <brking@...ux.vnet.ibm.com>
To:	"Philip J. Kelleher" <pjk1939@...ux.vnet.ibm.com>
CC:	axboe@...nel.dk, linux-kernel@...r.kernel.org,
	josh.h.morris@...ibm.com
Subject: Re: [PATCH 1/1] block: IBM RamSan 70/80 driver fixes.

On 02/18/2013 02:00 PM, Philip J. Kelleher wrote:
> diff -uprN -X linux-block-vanilla/Documentation/dontdiff linux-block-vanilla/drivers/block/rsxx/config.c linux-block/drivers/block/rsxx/config.c
> --- linux-block-vanilla/drivers/block/rsxx/config.c	2013-02-12 11:25:37.756352070 -0600
> +++ linux-block/drivers/block/rsxx/config.c	2013-02-15 09:01:43.221166194 -0600
> @@ -31,7 +31,7 @@
> 
>  static void initialize_config(void *config)
>  {
> -	struct rsxx_card_cfg *cfg = (struct rsxx_card_cfg *) config;
> +	struct rsxx_card_cfg *cfg = config;

Why not pass a struct rsxx_card_cfg * here instead of a void*?



> @@ -126,7 +126,11 @@ static void creg_issue_cmd(struct rsxx_c
>  					  cmd->buf, cmd->stream);
>  	}
> 
> -	/* Data copy must complete before initiating the command. */
> +	/*
> +	 * Data copy must complete before initiating the command. This is
> +	 * needed for weakly ordered processors (i.e. PowerPC), so that all
> +	 * neccessary registers are written before we kick the hardware.
> +	 */
>  	wmb();

When you say data copy are you referring to the writes to the host DMA
buffer that occurred previously? If so, the iowrite / writel macros
already ensure this, as they have a sync instruction built in to them
to cover this case, so a wmb would be redundant.

If its to ensure that all the iowrite's make it to the device as one
transaction and don't get interleaved with some other iowrite's, as
long as you have a spinlock protecting these writes, the PowerPC
spin_unlock will guarantee an mmiowb, so this shouldn't be an issue
either.


> diff -uprN -X linux-block-vanilla/Documentation/dontdiff linux-block-vanilla/drivers/block/rsxx/rsxx.h linux-block/drivers/block/rsxx/rsxx.h
> --- linux-block-vanilla/drivers/block/rsxx/rsxx.h	2013-02-12 11:25:37.780170779 -0600
> +++ linux-block/drivers/block/rsxx/rsxx.h	2013-02-18 08:54:59.692973434 -0600
> @@ -35,6 +35,8 @@ struct rsxx_reg_access {
>  	__u32 data[8];
>  };
> 
> +#define RSXX_MAX_REG_CNT	(8 * (sizeof(__u32)))

Is this 8 related to the 8 in the array above? If so, why not have a literal
to define the 8 and use it in both places to make this clear?

-Brian

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center


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