On Wed, Jan 26, 2011 at 3:30 PM, Dan Carpenter <[email protected]> wrote:
> Also when it does:
> memcpy(ft1000dev->tx_buf, *pUcFile, byte_length);
>
> That should probably be:
> memcpy(ft1000dev->tx_buf, *pUcFile, word_length * 4);
No this shouldn't because before you have additional check:
if (byte_length && ((byte_length % 64) == 0))
byte_length += 4;
if (byte_length < 64)
byte_length = 68;
So in my opinion byte_length should stay.
>
> Otherwise we're probably copying garbage from beyond the end of *pUcFile
> to the ->tx_buf. ft1000dev->tx_buf is hopefully initialized to zero at
> this point?
Yes usb_init_urb set it to zeroes.
>
> regards,
> dan carpenter
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
thanks,
marek
--
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer
Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
icq: 290551086
web: http://open-nandra.com
On Tue, Feb 08, 2011 at 02:40:49PM +0100, Belisko Marek wrote:
> On Wed, Jan 26, 2011 at 3:30 PM, Dan Carpenter <[email protected]> wrote:
> > Also when it does:
> > ? ? ? ?memcpy(ft1000dev->tx_buf, *pUcFile, byte_length);
> >
> > That should probably be:
> > ? ? ? ?memcpy(ft1000dev->tx_buf, *pUcFile, word_length * 4);
> No this shouldn't because before you have additional check:
> if (byte_length && ((byte_length % 64) == 0))
> byte_length += 4;
>
> if (byte_length < 64)
> byte_length = 68;
> So in my opinion byte_length should stay.
Yes. We make byte_length longer than the caller intended. The caller
knows the size of the source buffer. We have to pad the length of the
other buffer, but we should fill up the last part with zeroes instead
of reading past the end of the source buffer.
(I am not very familiar with the code and I haven't looked outside this
function, so I may be wrong).
Also I really bet that the thing where byte_length can't be a multiple
of 64 is bogus. I've never heard of anything with a requirement like
that.
regards,
dan carpenter
On Tue, Feb 8, 2011 at 5:35 PM, Dan Carpenter <[email protected]> wrote:
> On Tue, Feb 08, 2011 at 02:40:49PM +0100, Belisko Marek wrote:
>> On Wed, Jan 26, 2011 at 3:30 PM, Dan Carpenter <[email protected]> wrote:
>> > Also when it does:
>> > memcpy(ft1000dev->tx_buf, *pUcFile, byte_length);
>> >
>> > That should probably be:
>> > memcpy(ft1000dev->tx_buf, *pUcFile, word_length * 4);
>> No this shouldn't because before you have additional check:
>> if (byte_length && ((byte_length % 64) == 0))
>> byte_length += 4;
>>
>> if (byte_length < 64)
>> byte_length = 68;
>> So in my opinion byte_length should stay.
>
> Yes. We make byte_length longer than the caller intended. The caller
> knows the size of the source buffer. We have to pad the length of the
> other buffer, but we should fill up the last part with zeroes instead
> of reading past the end of the source buffer.
>
> (I am not very familiar with the code and I haven't looked outside this
> function, so I may be wrong).
>
> Also I really bet that the thing where byte_length can't be a multiple
> of 64 is bogus. I've never heard of anything with a requirement like
> that.
You're right. Today will make test when remove all opaque code.
Anyway at the end file position is moved in that way:
*pUsFile = *pUsFile + (word_length << 1);
*pUcFile = *pUcFile + (word_length << 2);
So short pointer multiplied by 2 and char pointer by 4 with
word_length. So my assume
all check and byte_length increasing is not correct (will see what test shows).
>
> regards,
> dan carpenter
>
>
>
regards,
marek
--
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer
Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
icq: 290551086
web: http://open-nandra.com
On Tue, Feb 8, 2011 at 5:35 PM, Dan Carpenter <[email protected]> wrote:
> On Tue, Feb 08, 2011 at 02:40:49PM +0100, Belisko Marek wrote:
>> On Wed, Jan 26, 2011 at 3:30 PM, Dan Carpenter <[email protected]> wrote:
>> > Also when it does:
>> > memcpy(ft1000dev->tx_buf, *pUcFile, byte_length);
>> >
>> > That should probably be:
>> > memcpy(ft1000dev->tx_buf, *pUcFile, word_length * 4);
>> No this shouldn't because before you have additional check:
>> if (byte_length && ((byte_length % 64) == 0))
>> byte_length += 4;
>>
>> if (byte_length < 64)
>> byte_length = 68;
>> So in my opinion byte_length should stay.
>
> Yes. We make byte_length longer than the caller intended. The caller
> knows the size of the source buffer. We have to pad the length of the
> other buffer, but we should fill up the last part with zeroes instead
> of reading past the end of the source buffer.
>
> (I am not very familiar with the code and I haven't looked outside this
> function, so I may be wrong).
>
> Also I really bet that the thing where byte_length can't be a multiple
> of 64 is bogus. I've never heard of anything with a requirement like
> that.
Well I test it and it seems very strange and can't figure out why.
Will remove all byte_length manipulations and device doesn't boot properly
(finish with error). Add some prinkt to code figure out following:
1. byte_length = word_length *4 is < 64 we need to send via usb 68
bytes (despite
4 bytes are behind 64 (without this it can't boot).
2. also when e.g. word_length is 400 (400*4 = 1600) condition
1600%64 == 0 is valid and we send 1604 bytes to usb (also not sure why
but without this change it also doesn't work).
For little explanation when we get to state code load we ask device
how many bytes should send
so we will get reply and send block via usb. So maybe it's related
that we send in our assumption correct data
but usb request something else to properly working ;)
>
> regards,
> dan carpenter
>
>
>
thanks,
marek
--
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer
Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
icq: 290551086
web: http://open-nandra.com
On Wed, Feb 09, 2011 at 11:07:19AM +0100, Belisko Marek wrote:
> Well I test it and it seems very strange and can't figure out why.
> Will remove all byte_length manipulations and device doesn't boot properly
> (finish with error). Add some prinkt to code figure out following:
>
> 1. byte_length = word_length *4 is < 64 we need to send via usb 68
> bytes (despite
> 4 bytes are behind 64 (without this it can't boot).
>
> 2. also when e.g. word_length is 400 (400*4 = 1600) condition
> 1600%64 == 0 is valid and we send 1604 bytes to usb (also not sure why
> but without this change it also doesn't work).
>
Huh. Strange. Thanks for testing. Sorry for the noise.
regards,
dan carpenter
On Wed, Feb 9, 2011 at 12:08 PM, Dan Carpenter <[email protected]> wrote:
> On Wed, Feb 09, 2011 at 11:07:19AM +0100, Belisko Marek wrote:
>> Well I test it and it seems very strange and can't figure out why.
>> Will remove all byte_length manipulations and device doesn't boot properly
>> (finish with error). Add some prinkt to code figure out following:
>>
>> 1. byte_length = word_length *4 is < 64 we need to send via usb 68
>> bytes (despite
>> 4 bytes are behind 64 (without this it can't boot).
>>
>> 2. also when e.g. word_length is 400 (400*4 = 1600) condition
>> 1600%64 == 0 is valid and we send 1604 bytes to usb (also not sure why
>> but without this change it also doesn't work).
>>
>
> Huh. Strange. Thanks for testing. Sorry for the noise.
No problem ;). Thanks for reviewing and making noise ;).
@greg: could you please apply posted patch(thanks):
http://driverdev.linuxdriverproject.org/pipermail/devel/2011-February/011942.html
>
> regards,
> dan carpenter
>
>
>
thanks,
marek
--
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer
Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
icq: 290551086
web: http://open-nandra.com