Re: [PATCH] parport: fix inverted daisy selection result check

[Marko Kohtala]

Hi.

My guess from this 20 year old commit 7c9cc3be1094 of mine is that I
had thought the response nError signal was read at the wrong time. The
return value was not intended to change when it was read at the
correct time, but it was inverted to keep it the same.

I don't have the IEEE 1284 specifications and datasheet available.
Here is what I think is happening: The parport_frob_control
PARPORT_CONTROL_STROBE bit is inverted into the bus signal. The
parport_read_status PARPORT_STATUS_ERROR bit is not inverted and shows
directly the bus signal. After the wake up sequence of aa, 55, 00, ff,
the PARPORT_STATUS_ERROR is verified to be 1. Again, after the 0x87 it
is verified to be 1. After the nStrobe signal goes low, the nError is
read for acknowledgement while nStrobe is still low. The command is
most likely acknowledged with a different PARPORT_STATUS_ERROR value
of 0, so the return value of parport_daisy_select is now 1 for a
success. I probably had a setup where parport_daisy_select had
returned PARPORT_STATUS_ERROR in the same situation due to reading the
bit after nStrobe was back up.

I only had one daisy chain device. It is possible the return value of
parport_daisy_select was changed and I did not notice the difference.

I think one could consider returning the parport_daisy_select return
value to the original value rather than spread the change further.
It's been like this for 20 years, so there could be some other work
depending on this.

I see the linux-parport@xxxxxxxxxxxxxxxxxxx is not in the recipients.
They might have an opinion on this.

Marko


ma 27.7.2026 klo 10.01 Yang Shao (shaoyang@xxxxxxxxxxxxx) kirjoitti:
>
> parport_daisy_select() returns a boolean success result. It returns 1
> when the requested daisy-chain device acknowledges the CPP selection
> command and 0 otherwise.
>
> Commit 7c9cc3be1094 ("[PATCH] parport: parport_daisy_select return
> value fix") changed the helper from returning the raw
> PARPORT_STATUS_ERROR bit to returning its logical negation. Selection
> success therefore changed from a zero return value to a nonzero return
> value, but parport_claim() retained the old zero-is-success test.
>
> As a result, a successful selection does not update port->daisy, while
> an unsuccessful selection can be recorded as the currently selected
> daisy address. parport_open() later checks port->daisy to determine
> whether the requested device was successfully selected, so this can
> reject an existing device or accept a device address that was not
> selected.
>
> Test the return value directly and update port->daisy only after a
> successful selection.
>
> Fixes: 7c9cc3be1094 ("[PATCH] parport: parport_daisy_select return value fix")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Yang Shao <shaoyang@xxxxxxxxxxxxx>
> ---
>  drivers/parport/share.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/parport/share.c b/drivers/parport/share.c
> index c4c5869..2888005 100644
> --- a/drivers/parport/share.c
> +++ b/drivers/parport/share.c
> @@ -1025,8 +1025,8 @@ int parport_claim(struct pardevice *dev)
>         /* If it's a daisy chain device, select it. */
>         if (dev->daisy >= 0) {
>                 /* This could be lazier. */
> -               if (!parport_daisy_select(port, dev->daisy,
> -                                          IEEE1284_MODE_COMPAT))
> +               if (parport_daisy_select(port, dev->daisy,
> +                                        IEEE1284_MODE_COMPAT))
>                         port->daisy = dev->daisy;
>         }
>  #endif /* IEEE1284.3 support */
> --
> 2.47.3
>