Patchwork [U-Boot,8/9] ums: always initialize mmc before ums_disk_init()

login
register
mail settings
Submitter Mateusz Zalega
Date Jan. 9, 2014, 2:31 p.m.
Message ID <1389277919-15279-8-git-send-email-m.zalega@samsung.com>
Download mbox | patch
Permalink /patch/308823/
State Changes Requested
Delegated to: Minkyu Kang
Headers show

Comments

Mateusz Zalega - Jan. 9, 2014, 2:31 p.m.
In some cases MMC was still uninitialized while media capacity check,
leading to broken ums command.

Change-Id: I4b86c2c59e430fb8b55272ea14f00316d8cb3dca
Signed-off-by: Mateusz Zalega <m.zalega@samsung.com>
Cc: Lukasz Majewski <l.majewski@samsung.com>
Cc: Minkyu Kang <mk7.kang@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
---
 board/samsung/common/ums.c | 3 +++
 1 file changed, 3 insertions(+)
Jaehoon Chung - Jan. 10, 2014, 5:08 a.m.
Hi Mateusz,

On 01/09/2014 11:31 PM, Mateusz Zalega wrote:
> In some cases MMC was still uninitialized while media capacity check,
> leading to broken ums command.
> 
> Change-Id: I4b86c2c59e430fb8b55272ea14f00316d8cb3dca
> Signed-off-by: Mateusz Zalega <m.zalega@samsung.com>
> Cc: Lukasz Majewski <l.majewski@samsung.com>
> Cc: Minkyu Kang <mk7.kang@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  board/samsung/common/ums.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/board/samsung/common/ums.c b/board/samsung/common/ums.c
> index dc155ad..0d8f30d 100644
> --- a/board/samsung/common/ums.c
> +++ b/board/samsung/common/ums.c
> @@ -37,6 +37,9 @@ static struct ums ums_dev = {
>  
>  static struct ums *ums_disk_init(struct mmc *mmc)
>  {
> +	if (mmc_init(mmc))
> +		return NULL;
> +
>  	uint64_t mmc_end_sector = mmc->capacity / SECTOR_SIZE;
>  	uint64_t ums_end_sector = UMS_NUM_SECTORS + UMS_START_SECTOR;

-->	if (mmc_init(mmc))
		return NULL;

Locate this point.

Best Regards,
Jaehoon Chung
>  
>
Łukasz Majewski - Jan. 13, 2014, 10:16 a.m.
Hi Mateusz,

> In some cases MMC was still uninitialized while media capacity check,
> leading to broken ums command.
> 
> Change-Id: I4b86c2c59e430fb8b55272ea14f00316d8cb3dca
> Signed-off-by: Mateusz Zalega <m.zalega@samsung.com>
> Cc: Lukasz Majewski <l.majewski@samsung.com>
> Cc: Minkyu Kang <mk7.kang@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  board/samsung/common/ums.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/board/samsung/common/ums.c b/board/samsung/common/ums.c
> index dc155ad..0d8f30d 100644
> --- a/board/samsung/common/ums.c
> +++ b/board/samsung/common/ums.c
> @@ -37,6 +37,9 @@ static struct ums ums_dev = {
>  
>  static struct ums *ums_disk_init(struct mmc *mmc)
>  {
> +	if (mmc_init(mmc))
> +		return NULL;
> +
>  	uint64_t mmc_end_sector = mmc->capacity / SECTOR_SIZE;
>  	uint64_t ums_end_sector = UMS_NUM_SECTORS + UMS_START_SECTOR;
>  

Tested-by: Lukasz Majewski <l.majewski@samsung.com>

Tested at: Exynos4210 - TRATS.
Mateusz Zalega - Jan. 13, 2014, 2:39 p.m.
On 01/10/14 06:08, Jaehoon Chung wrote:
>> index dc155ad..0d8f30d 100644
>> --- a/board/samsung/common/ums.c
>> +++ b/board/samsung/common/ums.c
>> @@ -37,6 +37,9 @@ static struct ums ums_dev = {
>>  
>>  static struct ums *ums_disk_init(struct mmc *mmc)
>>  {
>> +	if (mmc_init(mmc))
>> +		return NULL;
>> +
>>  	uint64_t mmc_end_sector = mmc->capacity / SECTOR_SIZE;
>>  	uint64_t ums_end_sector = UMS_NUM_SECTORS + UMS_START_SECTOR;
> 
> -->	if (mmc_init(mmc))
> 		return NULL;
> 
> Locate this point.

If you're asking to put this if() block after variable declaration, NAK.

It's perfectly fine C99 code. I'm not aware of any existing U-Boot style
guidelines that would forbid me to leave it this way.

These variables are only meaningful when mmc_init() returns a valid pointer.

Regards,
Michael Trimarchi - Jan. 13, 2014, 2:43 p.m.
Hi

On Mon, Jan 13, 2014 at 3:39 PM, Mateusz Zalega <m.zalega@samsung.com> wrote:
> On 01/10/14 06:08, Jaehoon Chung wrote:
>>> index dc155ad..0d8f30d 100644
>>> --- a/board/samsung/common/ums.c
>>> +++ b/board/samsung/common/ums.c
>>> @@ -37,6 +37,9 @@ static struct ums ums_dev = {
>>>
>>>  static struct ums *ums_disk_init(struct mmc *mmc)
>>>  {
>>> +    if (mmc_init(mmc))
>>> +            return NULL;
>>> +
>>>      uint64_t mmc_end_sector = mmc->capacity / SECTOR_SIZE;
>>>      uint64_t ums_end_sector = UMS_NUM_SECTORS + UMS_START_SECTOR;
>>
>> -->   if (mmc_init(mmc))
>>               return NULL;
>>
>> Locate this point.
>
> If you're asking to put this if() block after variable declaration, NAK.
>

I don't understand your point

> It's perfectly fine C99 code. I'm not aware of any existing U-Boot style
> guidelines that would forbid me to leave it this way.
>
> These variables are only meaningful when mmc_init() returns a valid pointer.
>

http://www.denx.de/wiki/U-Boot/CodingStyle

Michael

> Regards,
> --
> Mateusz Zalega
> Samsung R&D Institute Poland
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Mateusz Zalega - Jan. 13, 2014, 3 p.m.
On 01/13/14 15:43, Michael Trimarchi wrote:
> On Mon, Jan 13, 2014 at 3:39 PM, Mateusz Zalega <m.zalega@samsung.com> wrote:
>> On 01/10/14 06:08, Jaehoon Chung wrote:
>>>> index dc155ad..0d8f30d 100644
>>>> --- a/board/samsung/common/ums.c
>>>> +++ b/board/samsung/common/ums.c
>>>> @@ -37,6 +37,9 @@ static struct ums ums_dev = {
>>>>
>>>>  static struct ums *ums_disk_init(struct mmc *mmc)
>>>>  {
>>>> +    if (mmc_init(mmc))
>>>> +            return NULL;
>>>> +
>>>>      uint64_t mmc_end_sector = mmc->capacity / SECTOR_SIZE;
>>>>      uint64_t ums_end_sector = UMS_NUM_SECTORS + UMS_START_SECTOR;
>>>
>>> -->   if (mmc_init(mmc))
>>>               return NULL;
>>>
>>> Locate this point.
>>
>> If you're asking to put this if() block after variable declaration, NAK.
>>
> 
> I don't understand your point
> 
>> It's perfectly fine C99 code. I'm not aware of any existing U-Boot style
>> guidelines that would forbid me to leave it this way.
>>
>> These variables are only meaningful when mmc_init() returns a valid pointer.
>>
> 
> http://www.denx.de/wiki/U-Boot/CodingStyle
> 
> Michael

touché

OK, I'll move declarations to the beginning of the closure, C89 style.

Thanks,

Patch

diff --git a/board/samsung/common/ums.c b/board/samsung/common/ums.c
index dc155ad..0d8f30d 100644
--- a/board/samsung/common/ums.c
+++ b/board/samsung/common/ums.c
@@ -37,6 +37,9 @@  static struct ums ums_dev = {
 
 static struct ums *ums_disk_init(struct mmc *mmc)
 {
+	if (mmc_init(mmc))
+		return NULL;
+
 	uint64_t mmc_end_sector = mmc->capacity / SECTOR_SIZE;
 	uint64_t ums_end_sector = UMS_NUM_SECTORS + UMS_START_SECTOR;