Re: [PATCH 2/7] staging: fsl-mc: use generic memory barriers

[Laurentiu Tudor]

On 07/17/2017 04:38 PM, Robin Murphy wrote:
> On 17/07/17 14:26, laurentiu.tu...@nxp.com wrote:
>> From: Laurentiu Tudor 
>>
>> No need to use arch-specific memory barriers; switch to using generic
>> ones. The rmb()s were useless so drop them.
>>
>> Signed-off-by: Laurentiu Tudor 
>> ---
>>   drivers/staging/fsl-mc/bus/mc-sys.c | 6 ++
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/staging/fsl-mc/bus/mc-sys.c 
>> b/drivers/staging/fsl-mc/bus/mc-sys.c
>> index a1704c3..012abd5 100644
>> --- a/drivers/staging/fsl-mc/bus/mc-sys.c
>> +++ b/drivers/staging/fsl-mc/bus/mc-sys.c
>> @@ -127,7 +127,8 @@ static inline void mc_write_command(struct mc_command 
>> __iomem *portal,
>>  /* copy command parameters into the portal */
>>  for (i = 0; i < MC_CMD_NUM_OF_PARAMS; i++)
>>  __raw_writeq(cmd->params[i], >params[i]);
>> -__iowmb();
>> +/* ensure command params are committed before submitting it */
>> +wmb();
>>
>>  /* submit the command by writing the header */
>>  __raw_writeq(cmd->header, >header);
>
> AFAICS, just using writeq() here should ensure sufficient order against
> the previous iomem accessors, without the need for explicit barriers.

Endianess is handled in the callers, this function should leave the raw 
data unchanged. So the raw version was chosen on purpose.

> Also, note that the __raw_*() variants aren't endian-safe, so consider
> updating things to *_relaxed() where ordering doesn't matter.
>

That's what i tried in the first place but encountered compilation 
errors on 32-bit powerpc & 32-bit x86 having to do with writeq/readq 
variants not being available (IIRC). So that's why i switched to the 
32-bit variants in the end.

---
Thanks & Best Regards, Laurentiu

Re: [PATCH 2/7] staging: fsl-mc: use generic memory barriers

[Laurentiu Tudor]

On 07/17/2017 04:38 PM, Robin Murphy wrote:
> On 17/07/17 14:26, laurentiu.tu...@nxp.com wrote:
>> From: Laurentiu Tudor 
>>
>> No need to use arch-specific memory barriers; switch to using generic
>> ones. The rmb()s were useless so drop them.
>>
>> Signed-off-by: Laurentiu Tudor 
>> ---
>>   drivers/staging/fsl-mc/bus/mc-sys.c | 6 ++
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/staging/fsl-mc/bus/mc-sys.c 
>> b/drivers/staging/fsl-mc/bus/mc-sys.c
>> index a1704c3..012abd5 100644
>> --- a/drivers/staging/fsl-mc/bus/mc-sys.c
>> +++ b/drivers/staging/fsl-mc/bus/mc-sys.c
>> @@ -127,7 +127,8 @@ static inline void mc_write_command(struct mc_command 
>> __iomem *portal,
>>  /* copy command parameters into the portal */
>>  for (i = 0; i < MC_CMD_NUM_OF_PARAMS; i++)
>>  __raw_writeq(cmd->params[i], >params[i]);
>> -__iowmb();
>> +/* ensure command params are committed before submitting it */
>> +wmb();
>>
>>  /* submit the command by writing the header */
>>  __raw_writeq(cmd->header, >header);
>
> AFAICS, just using writeq() here should ensure sufficient order against
> the previous iomem accessors, without the need for explicit barriers.

Endianess is handled in the callers, this function should leave the raw 
data unchanged. So the raw version was chosen on purpose.

> Also, note that the __raw_*() variants aren't endian-safe, so consider
> updating things to *_relaxed() where ordering doesn't matter.
>

That's what i tried in the first place but encountered compilation 
errors on 32-bit powerpc & 32-bit x86 having to do with writeq/readq 
variants not being available (IIRC). So that's why i switched to the 
32-bit variants in the end.

---
Thanks & Best Regards, Laurentiu

Re: [PATCH 2/7] staging: fsl-mc: use generic memory barriers

[Robin Murphy]

On 17/07/17 14:26, laurentiu.tu...@nxp.com wrote:
> From: Laurentiu Tudor 
> 
> No need to use arch-specific memory barriers; switch to using generic
> ones. The rmb()s were useless so drop them.
> 
> Signed-off-by: Laurentiu Tudor 
> ---
>  drivers/staging/fsl-mc/bus/mc-sys.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/fsl-mc/bus/mc-sys.c 
> b/drivers/staging/fsl-mc/bus/mc-sys.c
> index a1704c3..012abd5 100644
> --- a/drivers/staging/fsl-mc/bus/mc-sys.c
> +++ b/drivers/staging/fsl-mc/bus/mc-sys.c
> @@ -127,7 +127,8 @@ static inline void mc_write_command(struct mc_command 
> __iomem *portal,
>   /* copy command parameters into the portal */
>   for (i = 0; i < MC_CMD_NUM_OF_PARAMS; i++)
>   __raw_writeq(cmd->params[i], >params[i]);
> - __iowmb();
> + /* ensure command params are committed before submitting it */
> + wmb();
>  
>   /* submit the command by writing the header */
>   __raw_writeq(cmd->header, >header);

AFAICS, just using writeq() here should ensure sufficient order against
the previous iomem accessors, without the need for explicit barriers.

Also, note that the __raw_*() variants aren't endian-safe, so consider
updating things to *_relaxed() where ordering doesn't matter.

Robin.

> @@ -150,9 +151,7 @@ static inline enum mc_cmd_status mc_read_response(struct 
> mc_command __iomem *
>   enum mc_cmd_status status;
>  
>   /* Copy command response header from MC portal: */
> - __iormb();
>   resp->header = __raw_readq(>header);
> - __iormb();
>   status = mc_cmd_hdr_read_status(resp);
>   if (status != MC_CMD_STATUS_OK)
>   return status;
> @@ -160,7 +159,6 @@ static inline enum mc_cmd_status mc_read_response(struct 
> mc_command __iomem *
>   /* Copy command response data from MC portal: */
>   for (i = 0; i < MC_CMD_NUM_OF_PARAMS; i++)
>   resp->params[i] = __raw_readq(>params[i]);
> - __iormb();
>  
>   return status;
>  }
> 

Re: [PATCH 2/7] staging: fsl-mc: use generic memory barriers

[Robin Murphy]

On 17/07/17 14:26, laurentiu.tu...@nxp.com wrote:
> From: Laurentiu Tudor 
> 
> No need to use arch-specific memory barriers; switch to using generic
> ones. The rmb()s were useless so drop them.
> 
> Signed-off-by: Laurentiu Tudor 
> ---
>  drivers/staging/fsl-mc/bus/mc-sys.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/fsl-mc/bus/mc-sys.c 
> b/drivers/staging/fsl-mc/bus/mc-sys.c
> index a1704c3..012abd5 100644
> --- a/drivers/staging/fsl-mc/bus/mc-sys.c
> +++ b/drivers/staging/fsl-mc/bus/mc-sys.c
> @@ -127,7 +127,8 @@ static inline void mc_write_command(struct mc_command 
> __iomem *portal,
>   /* copy command parameters into the portal */
>   for (i = 0; i < MC_CMD_NUM_OF_PARAMS; i++)
>   __raw_writeq(cmd->params[i], >params[i]);
> - __iowmb();
> + /* ensure command params are committed before submitting it */
> + wmb();
>  
>   /* submit the command by writing the header */
>   __raw_writeq(cmd->header, >header);

AFAICS, just using writeq() here should ensure sufficient order against
the previous iomem accessors, without the need for explicit barriers.

Also, note that the __raw_*() variants aren't endian-safe, so consider
updating things to *_relaxed() where ordering doesn't matter.

Robin.

> @@ -150,9 +151,7 @@ static inline enum mc_cmd_status mc_read_response(struct 
> mc_command __iomem *
>   enum mc_cmd_status status;
>  
>   /* Copy command response header from MC portal: */
> - __iormb();
>   resp->header = __raw_readq(>header);
> - __iormb();
>   status = mc_cmd_hdr_read_status(resp);
>   if (status != MC_CMD_STATUS_OK)
>   return status;
> @@ -160,7 +159,6 @@ static inline enum mc_cmd_status mc_read_response(struct 
> mc_command __iomem *
>   /* Copy command response data from MC portal: */
>   for (i = 0; i < MC_CMD_NUM_OF_PARAMS; i++)
>   resp->params[i] = __raw_readq(>params[i]);
> - __iormb();
>  
>   return status;
>  }
> 

[PATCH 2/7] staging: fsl-mc: use generic memory barriers

[laurentiu.tudor]

From: Laurentiu Tudor 

No need to use arch-specific memory barriers; switch to using generic
ones. The rmb()s were useless so drop them.

Signed-off-by: Laurentiu Tudor 
---
 drivers/staging/fsl-mc/bus/mc-sys.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/fsl-mc/bus/mc-sys.c 
b/drivers/staging/fsl-mc/bus/mc-sys.c
index a1704c3..012abd5 100644
--- a/drivers/staging/fsl-mc/bus/mc-sys.c
+++ b/drivers/staging/fsl-mc/bus/mc-sys.c
@@ -127,7 +127,8 @@ static inline void mc_write_command(struct mc_command 
__iomem *portal,
/* copy command parameters into the portal */
for (i = 0; i < MC_CMD_NUM_OF_PARAMS; i++)
__raw_writeq(cmd->params[i], >params[i]);
-   __iowmb();
+   /* ensure command params are committed before submitting it */
+   wmb();
 
/* submit the command by writing the header */
__raw_writeq(cmd->header, >header);
@@ -150,9 +151,7 @@ static inline enum mc_cmd_status mc_read_response(struct 
mc_command __iomem *
enum mc_cmd_status status;
 
/* Copy command response header from MC portal: */
-   __iormb();
resp->header = __raw_readq(>header);
-   __iormb();
status = mc_cmd_hdr_read_status(resp);
if (status != MC_CMD_STATUS_OK)
return status;
@@ -160,7 +159,6 @@ static inline enum mc_cmd_status mc_read_response(struct 
mc_command __iomem *
/* Copy command response data from MC portal: */
for (i = 0; i < MC_CMD_NUM_OF_PARAMS; i++)
resp->params[i] = __raw_readq(>params[i]);
-   __iormb();
 
return status;
 }
-- 
2.9.4

[PATCH 2/7] staging: fsl-mc: use generic memory barriers

[laurentiu.tudor]

From: Laurentiu Tudor 

No need to use arch-specific memory barriers; switch to using generic
ones. The rmb()s were useless so drop them.

Signed-off-by: Laurentiu Tudor 
---
 drivers/staging/fsl-mc/bus/mc-sys.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/fsl-mc/bus/mc-sys.c 
b/drivers/staging/fsl-mc/bus/mc-sys.c
index a1704c3..012abd5 100644
--- a/drivers/staging/fsl-mc/bus/mc-sys.c
+++ b/drivers/staging/fsl-mc/bus/mc-sys.c
@@ -127,7 +127,8 @@ static inline void mc_write_command(struct mc_command 
__iomem *portal,
/* copy command parameters into the portal */
for (i = 0; i < MC_CMD_NUM_OF_PARAMS; i++)
__raw_writeq(cmd->params[i], >params[i]);
-   __iowmb();
+   /* ensure command params are committed before submitting it */
+   wmb();
 
/* submit the command by writing the header */
__raw_writeq(cmd->header, >header);
@@ -150,9 +151,7 @@ static inline enum mc_cmd_status mc_read_response(struct 
mc_command __iomem *
enum mc_cmd_status status;
 
/* Copy command response header from MC portal: */
-   __iormb();
resp->header = __raw_readq(>header);
-   __iormb();
status = mc_cmd_hdr_read_status(resp);
if (status != MC_CMD_STATUS_OK)
return status;
@@ -160,7 +159,6 @@ static inline enum mc_cmd_status mc_read_response(struct 
mc_command __iomem *
/* Copy command response data from MC portal: */
for (i = 0; i < MC_CMD_NUM_OF_PARAMS; i++)
resp->params[i] = __raw_readq(>params[i]);
-   __iormb();
 
return status;
 }
-- 
2.9.4