2009-02-27 16:54:08

by Philipp Zabel

[permalink] [raw]
Subject: [PATCH] pcmcia: dtl1_cs: fix pcmcia_loop_config logic

pcmcia_loop_config returns 0 on success.

Signed-off-by: Philipp Zabel <[email protected]>
---
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


2009-02-28 22:16:51

by Philipp Zabel

[permalink] [raw]
Subject: Re: [PATCH] pcmcia: dtl1_cs: fix pcmcia_loop_config logic

Hi Marcel,

On Sat, Feb 28, 2009 at 10:35 PM, Marcel Holtmann <[email protected]> wro=
te:
> Hi Philipp,
>
>> pcmcia_loop_config returns 0 on success.
>>
>> Signed-off-by: Philipp Zabel <[email protected]>
>> ---
>> =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

2009-02-28 21:38:28

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] pcmcia: dtl1_cs: fix pcmcia_loop_config logic

Hi Philipp.

> > pcmcia_loop_config returns 0 on success.
> >
> > Signed-off-by: Philipp Zabel <[email protected]>
> > ---
> > 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



2009-02-28 21:35:37

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] pcmcia: dtl1_cs: fix pcmcia_loop_config logic

Hi Philipp,

> pcmcia_loop_config returns 0 on success.
>
> Signed-off-by: Philipp Zabel <[email protected]>
> ---
> 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



2009-02-27 22:35:30

by Philipp Zabel

[permalink] [raw]
Subject: Re: [PATCH] pcmcia: dtl1_cs: fix pcmcia_loop_config logic

Hi,

On Fri, Feb 27, 2009 at 5:54 PM, Philipp Zabel <[email protected]> wr=
ote:
> pcmcia_loop_config returns 0 on success.
>
> Signed-off-by: Philipp Zabel <[email protected]>
> ---
> =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

2009-07-27 10:30:29

by Wolfram Sang

[permalink] [raw]
Subject: Re: [PATCH] pcmcia: dtl1_cs: fix pcmcia_loop_config logic


> > > On Fri, Feb 27, 2009 at 5:54 PM, Philipp Zabel <[email protected]> wrote:
> > >> pcmcia_loop_config returns 0 on success.
> > >>
> > >> Signed-off-by: Philipp Zabel <[email protected]>
> > >> ---
> > >> 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 <[email protected]>
>
> 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/ |


Attachments:
(No filename) (1.72 kB)
signature.asc (197.00 B)
Digital signature
Download all attachments

2009-07-27 10:13:31

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] pcmcia: dtl1_cs: fix pcmcia_loop_config logic

Hi Wolfram,

> > On Fri, Feb 27, 2009 at 5:54 PM, Philipp Zabel <[email protected]> wrote:
> >> pcmcia_loop_config returns 0 on success.
> >>
> >> Signed-off-by: Philipp Zabel <[email protected]>
> >> ---
> >> 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 <[email protected]>

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

Acked-by: Marcel Holtmann <[email protected]>

Regards

Marcel



2009-07-27 09:58:31

by Wolfram Sang

[permalink] [raw]
Subject: Re: [PATCH] pcmcia: dtl1_cs: fix pcmcia_loop_config logic

Hi,

pHilipp Zabel wrote:
> Hi,
>
> On Fri, Feb 27, 2009 at 5:54 PM, Philipp Zabel <[email protected]> wrote:
>> pcmcia_loop_config returns 0 on success.
>>
>> Signed-off-by: Philipp Zabel <[email protected]>
>> ---
>> 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 <[email protected]>

Regards,

Wolfram

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