[PATCH] pcmcia: dtl1_cs: fix pcmcia_loop_config logic

[PATCH] pcmcia: dtl1_cs: fix pcmcia_loop_config logic

[Philipp Zabel]

pcmcia_loop_config returns 0 on success.

Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
---
 drivers/bluetooth/dtl1_cs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/bluetooth/dtl1_cs.c b/drivers/bluetooth/dtl1_cs.c
index 901bdd9..e0ee642 100644
--- a/drivers/bluetooth/dtl1_cs.c
+++ b/drivers/bluetooth/dtl1_cs.c
@@ -616,7 +616,7 @@ static int dtl1_config(struct pcmcia_device *link)
 
 	/* Look for a generic full-sized window */
 	link->io.NumPorts1 = 8;
-	if (!pcmcia_loop_config(link, dtl1_confcheck, NULL))
+	if (pcmcia_loop_config(link, dtl1_confcheck, NULL))
 		goto failed;
 
 	i = pcmcia_request_irq(link, &link->irq);
-- 
1.5.6.5

Re: [PATCH] pcmcia: dtl1_cs: fix pcmcia_loop_config logic

[pHilipp Zabel]

Hi,

On Fri, Feb 27, 2009 at 5:54 PM, Philipp Zabel <philipp.zabel@gmail.com> wr=
ote:
> pcmcia_loop_config returns 0 on success.
>
> Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
> ---
> =A0drivers/bluetooth/dtl1_cs.c | =A0 =A02 +-
> =A01 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/bluetooth/dtl1_cs.c b/drivers/bluetooth/dtl1_cs.c
> index 901bdd9..e0ee642 100644
> --- a/drivers/bluetooth/dtl1_cs.c
> +++ b/drivers/bluetooth/dtl1_cs.c
> @@ -616,7 +616,7 @@ static int dtl1_config(struct pcmcia_device *link)
>
> =A0 =A0 =A0 =A0/* Look for a generic full-sized window */
> =A0 =A0 =A0 =A0link->io.NumPorts1 =3D 8;
> - =A0 =A0 =A0 if (!pcmcia_loop_config(link, dtl1_confcheck, NULL))
> + =A0 =A0 =A0 if (pcmcia_loop_config(link, dtl1_confcheck, NULL))
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto failed;
>
> =A0 =A0 =A0 =A0i =3D pcmcia_request_irq(link, &link->irq);
> --
> 1.5.6.5

with this change my Nokia DTL-1 CF card gets detected, but the
interface stays down.

# hciconfig hci0 up
Can't init device hci0: Connection timed out (110)

I added some printk's to dtl_write and dtl_receive to dump the TX/RX
bytes and rx_count (in parentheses):

pcmcia 1.0: pcmcia: registering new device pcmcia1.0
dtl1_receive: 80(4) 10(3) 02(2) 00(1) 0f(2) 00(1) <6>
Bluetooth: Nokia control data =3D 0f 00
dtl1_write: 81 00 03 00 03 0c 00 00
dtl1_interrupt: RLSI
dtl1_receive: 00(4)
dtl1_receive: 80(3) 10(2) 02(1) 00(528) 0f(527) 00(526)
dtl1_receive: 84(525) 00(524) 06(523) 00(522) 0e(521) 04(520) 04(519) 03(51=
8)
dtl1_receive: 0c(517) 00(516)
dtl1_write: 81 00 03 00 03 0c 00 00
dtl1_interrupt: RLSI
dtl1_receive: 00(515)
dtl1_receive: 80(514) 10(513) 02(512) 00(511) 0f(510) 00(509)
dtl1_receive: 84(508) 00(507) 06(506) 00(505) 0e(504) 04(503) 04(502)
dtl1_receive: 03(501)
dtl1_receive: 0c(500)
dtl1_receive: 00(499)

It seems the culprit is the stray zero byte read after the RLSI
interrupt causing the 0x80 packet type to be interpreted as length. I
can work around this error with the following patch:

diff --git a/drivers/bluetooth/dtl1_cs.c b/drivers/bluetooth/dtl1_cs.c
index e0ee642..91c4c92 100644
--- a/drivers/bluetooth/dtl1_cs.c
+++ b/drivers/bluetooth/dtl1_cs.c
@@ -210,6 +210,7 @@ static void dtl1_receive(dtl1_info_t *info)
        unsigned int iobase;
        nsh_t *nsh;
        int boguscount =3D 0;
+       __u8 byte;

        if (!info) {
                BT_ERR("Unknown device");
@@ -230,10 +231,16 @@ static void dtl1_receive(dtl1_info_t *info)
                                return;
                        }

-               *skb_put(info->rx_skb, 1) =3D inb(iobase + UART_RX);
-               nsh =3D (nsh_t *)info->rx_skb->data;
+               byte =3D inb(iobase + UART_RX);

-               info->rx_count--;
+               if ((info->rx_state =3D=3D RECV_WAIT_NSH) &&
(info->rx_count =3D=3D NSHL) && (byte =3D=3D 0)) {
+                       continue;
+               } else {
+                       *skb_put(info->rx_skb, 1) =3D byte;
+                       nsh =3D (nsh_t *)info->rx_skb->data;
+
+                       info->rx_count--;
+               }

                if (info->rx_count =3D=3D 0) {

But I fear this just covers up an error somewhere else. Can this RSLI
error and zero byte read be avoided somehow?

regards
Philipp

Re: [PATCH] pcmcia: dtl1_cs: fix pcmcia_loop_config logic

[Marcel Holtmann]

Hi Philipp,

> pcmcia_loop_config returns 0 on success.
> 
> Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
> ---
>  drivers/bluetooth/dtl1_cs.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/bluetooth/dtl1_cs.c b/drivers/bluetooth/dtl1_cs.c
> index 901bdd9..e0ee642 100644
> --- a/drivers/bluetooth/dtl1_cs.c
> +++ b/drivers/bluetooth/dtl1_cs.c
> @@ -616,7 +616,7 @@ static int dtl1_config(struct pcmcia_device *link)
>  
>  	/* Look for a generic full-sized window */
>  	link->io.NumPorts1 = 8;
> -	if (!pcmcia_loop_config(link, dtl1_confcheck, NULL))
> +	if (pcmcia_loop_config(link, dtl1_confcheck, NULL))
>  		goto failed;
>  
>  	i = pcmcia_request_irq(link, &link->irq);

can you check the other Bluetooth PCMCIA drivers, too. One of the PCMCIA
subsystem changes might have screwed this up and affects more drivers
than expected.

Regards

Marcel

Re: [PATCH] pcmcia: dtl1_cs: fix pcmcia_loop_config logic

[Marcel Holtmann]

Hi Philipp.

> > pcmcia_loop_config returns 0 on success.
> >
> > Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
> > ---
> >  drivers/bluetooth/dtl1_cs.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/bluetooth/dtl1_cs.c b/drivers/bluetooth/dtl1_cs.c
> > index 901bdd9..e0ee642 100644
> > --- a/drivers/bluetooth/dtl1_cs.c
> > +++ b/drivers/bluetooth/dtl1_cs.c
> > @@ -616,7 +616,7 @@ static int dtl1_config(struct pcmcia_device *link)
> >
> >        /* Look for a generic full-sized window */
> >        link->io.NumPorts1 = 8;
> > -       if (!pcmcia_loop_config(link, dtl1_confcheck, NULL))
> > +       if (pcmcia_loop_config(link, dtl1_confcheck, NULL))
> >                goto failed;
> >
> >        i = pcmcia_request_irq(link, &link->irq);
> > --
> > 1.5.6.5
> 
> with this change my Nokia DTL-1 CF card gets detected, but the
> interface stays down.
> 
> # hciconfig hci0 up
> Can't init device hci0: Connection timed out (110)
> 
> I added some printk's to dtl_write and dtl_receive to dump the TX/RX
> bytes and rx_count (in parentheses):
> 
> pcmcia 1.0: pcmcia: registering new device pcmcia1.0
> dtl1_receive: 80(4) 10(3) 02(2) 00(1) 0f(2) 00(1) <6>
> Bluetooth: Nokia control data = 0f 00
> dtl1_write: 81 00 03 00 03 0c 00 00
> dtl1_interrupt: RLSI
> dtl1_receive: 00(4)
> dtl1_receive: 80(3) 10(2) 02(1) 00(528) 0f(527) 00(526)
> dtl1_receive: 84(525) 00(524) 06(523) 00(522) 0e(521) 04(520) 04(519) 03(518)
> dtl1_receive: 0c(517) 00(516)
> dtl1_write: 81 00 03 00 03 0c 00 00
> dtl1_interrupt: RLSI
> dtl1_receive: 00(515)
> dtl1_receive: 80(514) 10(513) 02(512) 00(511) 0f(510) 00(509)
> dtl1_receive: 84(508) 00(507) 06(506) 00(505) 0e(504) 04(503) 04(502)
> dtl1_receive: 03(501)
> dtl1_receive: 0c(500)
> dtl1_receive: 00(499)
> 
> It seems the culprit is the stray zero byte read after the RLSI
> interrupt causing the 0x80 packet type to be interpreted as length. I
> can work around this error with the following patch:
> 
> diff --git a/drivers/bluetooth/dtl1_cs.c b/drivers/bluetooth/dtl1_cs.c
> index e0ee642..91c4c92 100644
> --- a/drivers/bluetooth/dtl1_cs.c
> +++ b/drivers/bluetooth/dtl1_cs.c
> @@ -210,6 +210,7 @@ static void dtl1_receive(dtl1_info_t *info)
>         unsigned int iobase;
>         nsh_t *nsh;
>         int boguscount = 0;
> +       __u8 byte;
> 
>         if (!info) {
>                 BT_ERR("Unknown device");
> @@ -230,10 +231,16 @@ static void dtl1_receive(dtl1_info_t *info)
>                                 return;
>                         }
> 
> -               *skb_put(info->rx_skb, 1) = inb(iobase + UART_RX);
> -               nsh = (nsh_t *)info->rx_skb->data;
> +               byte = inb(iobase + UART_RX);
> 
> -               info->rx_count--;
> +               if ((info->rx_state == RECV_WAIT_NSH) &&
> (info->rx_count == NSHL) && (byte == 0)) {
> +                       continue;
> +               } else {
> +                       *skb_put(info->rx_skb, 1) = byte;
> +                       nsh = (nsh_t *)info->rx_skb->data;
> +
> +                       info->rx_count--;
> +               }
> 
>                 if (info->rx_count == 0) {
> 
> But I fear this just covers up an error somewhere else. Can this RSLI
> error and zero byte read be avoided somehow?

The dtl1 driver is more reverse engineered than that we really know how
this Nokia protocol works. I would have to check with my DTL-1 and DTL-4
cards to see if this a general problem. However that has to wait until
April when I can pick them up again.

Regards

Marcel

Re: [PATCH] pcmcia: dtl1_cs: fix pcmcia_loop_config logic

[pHilipp Zabel]

Hi Marcel,

On Sat, Feb 28, 2009 at 10:35 PM, Marcel Holtmann <marcel@holtmann.org> wro=
te:
> Hi Philipp,
>
>> pcmcia_loop_config returns 0 on success.
>>
>> Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
>> ---
>> =A0drivers/bluetooth/dtl1_cs.c | =A0 =A02 +-
>> =A01 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/bluetooth/dtl1_cs.c b/drivers/bluetooth/dtl1_cs.c
>> index 901bdd9..e0ee642 100644
>> --- a/drivers/bluetooth/dtl1_cs.c
>> +++ b/drivers/bluetooth/dtl1_cs.c
>> @@ -616,7 +616,7 @@ static int dtl1_config(struct pcmcia_device *link)
>>
>> =A0 =A0 =A0 /* Look for a generic full-sized window */
>> =A0 =A0 =A0 link->io.NumPorts1 =3D 8;
>> - =A0 =A0 if (!pcmcia_loop_config(link, dtl1_confcheck, NULL))
>> + =A0 =A0 if (pcmcia_loop_config(link, dtl1_confcheck, NULL))
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto failed;
>>
>> =A0 =A0 =A0 i =3D pcmcia_request_irq(link, &link->irq);
>
> can you check the other Bluetooth PCMCIA drivers, too.

bluecard_cs doesn't use pcmcia_loop_config. bt3c_cs and btuart_cs both
have something like
	if (!pcmcia_loop_config(...))
		goto found_port;
instead, which looks fine.

> One of the PCMCIA
> subsystem changes might have screwed this up and affects more drivers
> than expected.

"pcmcia: use pcmcia_loop_config in bluetooth drivers"
(ed58872aa33e16a0d5352080e47c65fa14e6ad1c)
only touched bt3_cs, btuart_cs and dtl1_cs.

regards
Philipp

Re: [PATCH] pcmcia: dtl1_cs: fix pcmcia_loop_config logic

[Wolfram Sang]

Hi,

pHilipp Zabel wrote:
> Hi,
> 
> On Fri, Feb 27, 2009 at 5:54 PM, Philipp Zabel <philipp.zabel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> pcmcia_loop_config returns 0 on success.
>>
>> Signed-off-by: Philipp Zabel <philipp.zabel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/bluetooth/dtl1_cs.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/bluetooth/dtl1_cs.c b/drivers/bluetooth/dtl1_cs.c
>> index 901bdd9..e0ee642 100644
>> --- a/drivers/bluetooth/dtl1_cs.c
>> +++ b/drivers/bluetooth/dtl1_cs.c
>> @@ -616,7 +616,7 @@ static int dtl1_config(struct pcmcia_device *link)
>>
>>        /* Look for a generic full-sized window */
>>        link->io.NumPorts1 = 8;
>> -       if (!pcmcia_loop_config(link, dtl1_confcheck, NULL))
>> +       if (pcmcia_loop_config(link, dtl1_confcheck, NULL))
>>                goto failed;
>>
>>        i = pcmcia_request_irq(link, &link->irq);
>> --
>> 1.5.6.5
> 
> with this change my Nokia DTL-1 CF card gets detected, but the
> interface stays down.

I am reviewing patches which came through the pcmcia-list since the last 
upstream commit last year.

IMHO the above patch could be queued, even though there seem to be further 
issues with the driver. What do you think?

So, for the above patch:

Acked-by: Wolfram Sang <w.sang@pengutronix.de>

Regards,

    Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] pcmcia: dtl1_cs: fix pcmcia_loop_config logic

[Marcel Holtmann]

Hi Wolfram,

> > On Fri, Feb 27, 2009 at 5:54 PM, Philipp Zabel <philipp.zabel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> pcmcia_loop_config returns 0 on success.
> >>
> >> Signed-off-by: Philipp Zabel <philipp.zabel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >>  drivers/bluetooth/dtl1_cs.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/bluetooth/dtl1_cs.c b/drivers/bluetooth/dtl1_cs.c
> >> index 901bdd9..e0ee642 100644
> >> --- a/drivers/bluetooth/dtl1_cs.c
> >> +++ b/drivers/bluetooth/dtl1_cs.c
> >> @@ -616,7 +616,7 @@ static int dtl1_config(struct pcmcia_device *link)
> >>
> >>        /* Look for a generic full-sized window */
> >>        link->io.NumPorts1 = 8;
> >> -       if (!pcmcia_loop_config(link, dtl1_confcheck, NULL))
> >> +       if (pcmcia_loop_config(link, dtl1_confcheck, NULL))
> >>                goto failed;
> >>
> >>        i = pcmcia_request_irq(link, &link->irq);
> >> --
> >> 1.5.6.5
> > 
> > with this change my Nokia DTL-1 CF card gets detected, but the
> > interface stays down.
> 
> I am reviewing patches which came through the pcmcia-list since the last 
> upstream commit last year.
> 
> IMHO the above patch could be queued, even though there seem to be further 
> issues with the driver. What do you think?
> 
> So, for the above patch:
> 
> Acked-by: Wolfram Sang <w.sang@pengutronix.de>

can we change it into if (pcmcia_loop_config(...) < 0) to it perfectly
clear here.

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH] pcmcia: dtl1_cs: fix pcmcia_loop_config logic

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1810 bytes --]


> > > On Fri, Feb 27, 2009 at 5:54 PM, Philipp Zabel <philipp.zabel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > >> pcmcia_loop_config returns 0 on success.
> > >>
> > >> Signed-off-by: Philipp Zabel <philipp.zabel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > >> ---
> > >>  drivers/bluetooth/dtl1_cs.c |    2 +-
> > >>  1 files changed, 1 insertions(+), 1 deletions(-)
> > >>
> > >> diff --git a/drivers/bluetooth/dtl1_cs.c b/drivers/bluetooth/dtl1_cs.c
> > >> index 901bdd9..e0ee642 100644
> > >> --- a/drivers/bluetooth/dtl1_cs.c
> > >> +++ b/drivers/bluetooth/dtl1_cs.c
> > >> @@ -616,7 +616,7 @@ static int dtl1_config(struct pcmcia_device *link)
> > >>
> > >>        /* Look for a generic full-sized window */
> > >>        link->io.NumPorts1 = 8;
> > >> -       if (!pcmcia_loop_config(link, dtl1_confcheck, NULL))
> > >> +       if (pcmcia_loop_config(link, dtl1_confcheck, NULL))
> > >>                goto failed;
> > >>
> > >>        i = pcmcia_request_irq(link, &link->irq);
> > >> --
> > >> 1.5.6.5
> > > 
> > > with this change my Nokia DTL-1 CF card gets detected, but the
> > > interface stays down.
> > 
> > I am reviewing patches which came through the pcmcia-list since the last 
> > upstream commit last year.
> > 
> > IMHO the above patch could be queued, even though there seem to be further 
> > issues with the driver. What do you think?
> > 
> > So, for the above patch:
> > 
> > Acked-by: Wolfram Sang <w.sang@pengutronix.de>
> 
> can we change it into if (pcmcia_loop_config(...) < 0) to it perfectly
> clear here.

Yup, I'd like that better, too.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]