[RFC][PATCH][MTD][OneNAND] Fix OneNAND byte access

[RFC][PATCH][MTD][OneNAND] Fix OneNAND byte access

[Kyungmin Park]

Some upper layer try to read unaligned offset access
So it adjusts the buffer, offset, and count variables

/ # mount -t ubifs ubi0 /mnt
UBIFS: recovery needed
onenand_read_bufferram[528] 0 11	<- Unaligned count
onenand_read_bufferram[514] 91 12	<- Unaligned offset
onenand_read_bufferram[528] 92 11
onenand_read_bufferram[528] 74 17
onenand_read_bufferram[514] 103 17	<- Unaligned offset
onenand_read_bufferram[514] 11 17
onenand_read_bufferram[514] 137 17
onenand_read_bufferram[528] 120 17
onenand_read_bufferram[528] 40 17
onenand_read_bufferram[514] 57 17
onenand_read_bufferram[528] 632 61
onenand_read_bufferram[528] 360 61
onenand_read_bufferram[528] 80 61

It's only optimization at driver level

I think it's the best that it handles at UBIFS itself
e.g., Now it passed down from name handling
If the size of name is odd how about pad it even?

Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index 5d7965f..49194b6 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -509,9 +509,23 @@ static int onenand_read_bufferram(struct mtd_info *mtd, int area,
 
 	bufferram += onenand_bufferram_offset(mtd, area);
 
+	if (ONENAND_CHECK_BYTE_ACCESS(offset)) {
+		unsigned short word;
+		printk("%s[%d] %d %zd\n", __func__, __LINE__, offset, count);
+
+		/* Align with word(16-bit) size */
+		/* Read word and save byte */
+		word = this->read_word(bufferram + offset - 1);
+		buffer[0] = (word & 0xff00) >> 8;
+		buffer++;
+		offset++;
+		count--;
+	}
+
 	if (ONENAND_CHECK_BYTE_ACCESS(count)) {
 		unsigned short word;
 
+		printk("%s[%d] %d %zd\n", __func__, __LINE__, offset, count);
 		/* Align with word(16-bit) size */
 		count--;
 

Re: [RFC][PATCH][MTD][OneNAND] Fix OneNAND byte access

[Adrian Hunter]

Kyungmin Park wrote:
> Some upper layer try to read unaligned offset access
> So it adjusts the buffer, offset, and count variables
> 
> / # mount -t ubifs ubi0 /mnt
> UBIFS: recovery needed
> onenand_read_bufferram[528] 0 11	<- Unaligned count
> onenand_read_bufferram[514] 91 12	<- Unaligned offset
> onenand_read_bufferram[528] 92 11
> onenand_read_bufferram[528] 74 17
> onenand_read_bufferram[514] 103 17	<- Unaligned offset
> onenand_read_bufferram[514] 11 17
> onenand_read_bufferram[514] 137 17
> onenand_read_bufferram[528] 120 17
> onenand_read_bufferram[528] 40 17
> onenand_read_bufferram[514] 57 17
> onenand_read_bufferram[528] 632 61
> onenand_read_bufferram[528] 360 61
> onenand_read_bufferram[528] 80 61
> 
> It's only optimization at driver level
> 
> I think it's the best that it handles at UBIFS itself
> e.g., Now it passed down from name handling
> If the size of name is odd how about pad it even?

It would be a lot of work to change UBIFS.

It is really a driver problem.  For example OMAP2
does not have the problem, so it should be fixed at
the driver level.

> 
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
> index 5d7965f..49194b6 100644
> --- a/drivers/mtd/onenand/onenand_base.c
> +++ b/drivers/mtd/onenand/onenand_base.c
> @@ -509,9 +509,23 @@ static int onenand_read_bufferram(struct mtd_info *mtd, int area,
>  
>  	bufferram += onenand_bufferram_offset(mtd, area);
>  
> +	if (ONENAND_CHECK_BYTE_ACCESS(offset)) {
> +		unsigned short word;
> +		printk("%s[%d] %d %zd\n", __func__, __LINE__, offset, count);
> +
> +		/* Align with word(16-bit) size */
> +		/* Read word and save byte */
> +		word = this->read_word(bufferram + offset - 1);
> +		buffer[0] = (word & 0xff00) >> 8;
> +		buffer++;
> +		offset++;
> +		count--;
> +	}
> +
>  	if (ONENAND_CHECK_BYTE_ACCESS(count)) {
>  		unsigned short word;
>  
> +		printk("%s[%d] %d %zd\n", __func__, __LINE__, offset, count);
>  		/* Align with word(16-bit) size */
>  		count--;
>  
> 

I presume you will remove the printks.

Re: [RFC][PATCH][MTD][OneNAND] Fix OneNAND byte access

[Kyungmin Park]

>> It's only optimization at driver level
>>
>> I think it's the best that it handles at UBIFS itself
>> e.g., Now it passed down from name handling
>> If the size of name is odd how about pad it even?
>
> It would be a lot of work to change UBIFS.
>
> It is really a driver problem.  For example OMAP2
> does not have the problem, so it should be fixed at
> the driver level.
>

I'm not sure it makes a problem but potentially it access wrongly.
Well I will check it at omap2 driver

>>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>> diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
>> index 5d7965f..49194b6 100644
>> --- a/drivers/mtd/onenand/onenand_base.c
>> +++ b/drivers/mtd/onenand/onenand_base.c
>> @@ -509,9 +509,23 @@ static int onenand_read_bufferram(struct mtd_info *mtd, int area,
>>
>>       bufferram += onenand_bufferram_offset(mtd, area);
>>
>> +     if (ONENAND_CHECK_BYTE_ACCESS(offset)) {
>> +             unsigned short word;
>> +             printk("%s[%d] %d %zd\n", __func__, __LINE__, offset, count);
>> +
>> +             /* Align with word(16-bit) size */
>> +             /* Read word and save byte */
>> +             word = this->read_word(bufferram + offset - 1);
>> +             buffer[0] = (word & 0xff00) >> 8;
>> +             buffer++;
>> +             offset++;
>> +             count--;
>> +     }
>> +
>>       if (ONENAND_CHECK_BYTE_ACCESS(count)) {
>>               unsigned short word;
>>
>> +             printk("%s[%d] %d %zd\n", __func__, __LINE__, offset, count);
>>               /* Align with word(16-bit) size */
>>               count--;
>>
>>
>
> I presume you will remove the printks.
>

Sure, it remains only for checking for you. Must be removed at patch.

Thank you,
Kyungmin Park