alauda_check_media() doubts

alauda_check_media() doubts

[Oliver Neukum]

Hi Alan,

as you did something on this driver, doesn't
this condition:

(status[0] & 0x80) ||
                 ((status[0] & 0x1F) == 0x10) || ((status[1] & 0x01) == 0)

look odd to you? Especially the parentheses?

	Regards
		Oliver

Re: alauda_check_media() doubts

[Alan Stern]

On Tue, Aug 29, 2023 at 08:14:05PM +0200, Oliver Neukum wrote:
> Hi Alan,
> 
> as you did something on this driver, doesn't
> this condition:
> 
> (status[0] & 0x80) ||
>                 ((status[0] & 0x1F) == 0x10) || ((status[1] & 0x01) == 0)
> 
> look odd to you? Especially the parentheses?

(The actual text in my copy of the file is:

	if ((status[0] & 0x80) || ((status[0] & 0x1F) == 0x10)
		|| ((status[1] & 0x01) == 0)) {

This probably doesn't affect your point...)

Certainly the layout is a little peculiar, and the extra parentheses 
don't help any.  But they don't really hurt, either, and the meaning is 
clear.  It doesn't look obviously wrong.

Those two lines go back to the original version of the driver, added in 
2005 by commit e80b0fade09e ("[PATCH] USB Storage: add alauda support"), 
written by Daniel Drake and edited by Matt Dharm.  So it's been around 
for quite a while and there may not be many devices left that need this 
driver.

Did you want to change it?

Alan Stern

Re: alauda_check_media() doubts

[Oliver Neukum]

On 29.08.23 20:49, Alan Stern wrote:
> On Tue, Aug 29, 2023 at 08:14:05PM +0200, Oliver Neukum wrote:
>> Hi Alan,
>>
>> as you did something on this driver, doesn't
>> this condition:
>>
>> (status[0] & 0x80) ||
>>                  ((status[0] & 0x1F) == 0x10) || ((status[1] & 0x01) == 0)
>>
>> look odd to you? Especially the parentheses?
> 
> (The actual text in my copy of the file is:

Yes, I rearranged it by the parantheses.

> 	if ((status[0] & 0x80) || ((status[0] & 0x1F) == 0x10)
> 		|| ((status[1] & 0x01) == 0)) {
> 
> This probably doesn't affect your point...)
> 
> Certainly the layout is a little peculiar, and the extra parentheses
> don't help any.  But they don't really hurt, either, and the meaning is
> clear.  It doesn't look obviously wrong.

Ok, then this is just me. THe parantheses would make perfect sense
if the actual intent were:

(status[0] & 0x80) ||
		((status[0] & 0x1F) == 0x10) && ((status[1] & 0x01) == 0)
> 
> Those two lines go back to the original version of the driver, added in
> 2005 by commit e80b0fade09e ("[PATCH] USB Storage: add alauda support"),
> written by Daniel Drake and edited by Matt Dharm.  So it's been around
> for quite a while and there may not be many devices left that need this
> driver.

Yes, I know. Hence my question.

> Did you want to change it?

Nope. I just looked through the log and saw your patch for the
failure of the transfer and the subsequent test looked
messed up.

	Regards
		Oliver

Re: alauda_check_media() doubts

[Alan Stern]

On Tue, Aug 29, 2023 at 11:42:07PM +0200, Oliver Neukum wrote:
> 
> 
> On 29.08.23 20:49, Alan Stern wrote:
> > On Tue, Aug 29, 2023 at 08:14:05PM +0200, Oliver Neukum wrote:
> > > Hi Alan,
> > > 
> > > as you did something on this driver, doesn't
> > > this condition:
> > > 
> > > (status[0] & 0x80) ||
> > >                  ((status[0] & 0x1F) == 0x10) || ((status[1] & 0x01) == 0)
> > > 
> > > look odd to you? Especially the parentheses?
> > 
> > (The actual text in my copy of the file is:
> 
> Yes, I rearranged it by the parantheses.
> 
> > 	if ((status[0] & 0x80) || ((status[0] & 0x1F) == 0x10)
> > 		|| ((status[1] & 0x01) == 0)) {
> > 
> > This probably doesn't affect your point...)
> > 
> > Certainly the layout is a little peculiar, and the extra parentheses
> > don't help any.  But they don't really hurt, either, and the meaning is
> > clear.  It doesn't look obviously wrong.
> 
> Ok, then this is just me. THe parantheses would make perfect sense
> if the actual intent were:
> 
> (status[0] & 0x80) ||
> 		((status[0] & 0x1F) == 0x10) && ((status[1] & 0x01) == 0)

In fact, if I were writing that expression, I would do:

(status[0] & 0x80) ||
		((status[0] & 0x1F) == 0x10 && (status[1] & 0x01) == 0)

simply because I don't like relying on the relative precedence of || and 
&&.  That's just my own neurosis.  (On the other hand I have no qualms 
about the relative precedence of && and ==, because that combination of 
operators gets used all the time.  Maybe Daniel Drake preferred not to 
rely on it?)

For all we know, this is what the code _should_ be.  Without any 
documentation on the meaning of the status bits, there's no way to tell.

> > Those two lines go back to the original version of the driver, added in
> > 2005 by commit e80b0fade09e ("[PATCH] USB Storage: add alauda support"),
> > written by Daniel Drake and edited by Matt Dharm.  So it's been around
> > for quite a while and there may not be many devices left that need this
> > driver.
> 
> Yes, I know. Hence my question.
> 
> > Did you want to change it?
> 
> Nope. I just looked through the log and saw your patch for the
> failure of the transfer and the subsequent test looked
> messed up.

At this point, I don't think it matters much.

Alan Stern

Re: alauda_check_media() doubts

[Pawlicker]

It's interesting that there's semi active discussion about this driver.

I ran into the driver being completely and utterly broken today when I 
bought a Fujifilm DPC-R1 card reader (that was unsold for ages at the 
local camera store to read out my Minolta α-9's exposure info) that uses 
this chipset. It will either generate an oops error or media error 
depending on when the card is inserted into the reader, but it at least 
detects it.

I've submitted a bug (#217862) about this issue. The behavior is a bit 
interesting on older kernels (as in 5.15) as it simply generates the 
"transfer buffer is on stack" error but does not detect any media in the 
slot whatsoever or attempt to.

Jake (aka Pawlicker)

On 8/29/2023 7:28 PM, Alan Stern wrote:
> On Tue, Aug 29, 2023 at 11:42:07PM +0200, Oliver Neukum wrote:
>>
>>
>> On 29.08.23 20:49, Alan Stern wrote:
>>> On Tue, Aug 29, 2023 at 08:14:05PM +0200, Oliver Neukum wrote:
>>>> Hi Alan,
>>>>
>>>> as you did something on this driver, doesn't
>>>> this condition:
>>>>
>>>> (status[0] & 0x80) ||
>>>>                   ((status[0] & 0x1F) == 0x10) || ((status[1] & 0x01) == 0)
>>>>
>>>> look odd to you? Especially the parentheses?
>>>
>>> (The actual text in my copy of the file is:
>>
>> Yes, I rearranged it by the parantheses.
>>
>>> 	if ((status[0] & 0x80) || ((status[0] & 0x1F) == 0x10)
>>> 		|| ((status[1] & 0x01) == 0)) {
>>>
>>> This probably doesn't affect your point...)
>>>
>>> Certainly the layout is a little peculiar, and the extra parentheses
>>> don't help any.  But they don't really hurt, either, and the meaning is
>>> clear.  It doesn't look obviously wrong.
>>
>> Ok, then this is just me. THe parantheses would make perfect sense
>> if the actual intent were:
>>
>> (status[0] & 0x80) ||
>> 		((status[0] & 0x1F) == 0x10) && ((status[1] & 0x01) == 0)
> 
> In fact, if I were writing that expression, I would do:
> 
> (status[0] & 0x80) ||
> 		((status[0] & 0x1F) == 0x10 && (status[1] & 0x01) == 0)
> 
> simply because I don't like relying on the relative precedence of || and
> &&.  That's just my own neurosis.  (On the other hand I have no qualms
> about the relative precedence of && and ==, because that combination of
> operators gets used all the time.  Maybe Daniel Drake preferred not to
> rely on it?)
> 
> For all we know, this is what the code _should_ be.  Without any
> documentation on the meaning of the status bits, there's no way to tell.
> 
>>> Those two lines go back to the original version of the driver, added in
>>> 2005 by commit e80b0fade09e ("[PATCH] USB Storage: add alauda support"),
>>> written by Daniel Drake and edited by Matt Dharm.  So it's been around
>>> for quite a while and there may not be many devices left that need this
>>> driver.
>>
>> Yes, I know. Hence my question.
>>
>>> Did you want to change it?
>>
>> Nope. I just looked through the log and saw your patch for the
>> failure of the transfer and the subsequent test looked
>> messed up.
> 
> At this point, I don't think it matters much.
> 
> Alan Stern