[PATCH] mISDN cleanup user interface

[PATCH] mISDN cleanup user interface

[Karsten Keil]

Please pull from git://git.kernel.org/pub/scm/linux/kernel/git/kkeil/ISDN-2.6.git master

The channelmap should have the same size on 32 and 64 bit
systems. Thanks to David Woodhouse for spotting this.

Signed-off-by: Karsten Keil <kkeil@suse.de>
---
 drivers/isdn/hardware/mISDN/hfcmulti.c |    6 +++---
 drivers/isdn/hardware/mISDN/hfcpci.c   |    2 +-
 drivers/isdn/mISDN/l1oip_core.c        |    4 +++-
 drivers/isdn/mISDN/socket.c            |    4 ++--
 include/linux/mISDNif.h                |    7 ++++---
 5 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/isdn/hardware/mISDN/hfcmulti.c b/drivers/isdn/hardware/mISDN/hfcmulti.c
index 2649ea5..6449ffa 100644
--- a/drivers/isdn/hardware/mISDN/hfcmulti.c
+++ b/drivers/isdn/hardware/mISDN/hfcmulti.c
@@ -3971,7 +3971,7 @@ open_bchannel(struct hfc_multi *hc, struct dchannel *dch,
 	struct bchannel	*bch;
 	int		ch;
 
-	if (!test_bit(rq->adr.channel, &dch->dev.channelmap[0]))
+	if (!test_bit(rq->adr.channel, (u_long *)&dch->dev.channelmap[0]))
 		return -EINVAL;
 	if (rq->protocol == ISDN_P_NONE)
 		return -EINVAL;
@@ -4587,7 +4587,7 @@ init_e1_port(struct hfc_multi *hc, struct hm_map *m)
 		list_add(&bch->ch.list, &dch->dev.bchannels);
 		hc->chan[ch].bch = bch;
 		hc->chan[ch].port = 0;
-		test_and_set_bit(bch->nr, &dch->dev.channelmap[0]);
+		test_and_set_bit(bch->nr, (u_long *)&dch->dev.channelmap[0]);
 	}
 	/* set optical line type */
 	if (port[Port_cnt] & 0x001) {
@@ -4755,7 +4755,7 @@ init_multi_port(struct hfc_multi *hc, int pt)
 		list_add(&bch->ch.list, &dch->dev.bchannels);
 		hc->chan[i + ch].bch = bch;
 		hc->chan[i + ch].port = pt;
-		test_and_set_bit(bch->nr, &dch->dev.channelmap[0]);
+		test_and_set_bit(bch->nr, (u_long *)&dch->dev.channelmap[0]);
 	}
 	/* set master clock */
 	if (port[Port_cnt] & 0x001) {
diff --git a/drivers/isdn/hardware/mISDN/hfcpci.c b/drivers/isdn/hardware/mISDN/hfcpci.c
index 3231814..b111179 100644
--- a/drivers/isdn/hardware/mISDN/hfcpci.c
+++ b/drivers/isdn/hardware/mISDN/hfcpci.c
@@ -2056,7 +2056,7 @@ setup_card(struct hfc_pci *card)
 	card->dch.dev.nrbchan = 2;
 	for (i = 0; i < 2; i++) {
 		card->bch[i].nr = i + 1;
-		test_and_set_bit(i + 1, &card->dch.dev.channelmap[0]);
+		test_and_set_bit(i + 1, (u_long *)&card->dch.dev.channelmap[0]);
 		card->bch[i].debug = debug;
 		mISDN_initbchannel(&card->bch[i], MAX_DATA_MEM);
 		card->bch[i].hw = card;
diff --git a/drivers/isdn/mISDN/l1oip_core.c b/drivers/isdn/mISDN/l1oip_core.c
index 155b997..6cc7fd3 100644
--- a/drivers/isdn/mISDN/l1oip_core.c
+++ b/drivers/isdn/mISDN/l1oip_core.c
@@ -1006,8 +1006,10 @@ open_bchannel(struct l1oip *hc, struct dchannel *dch, struct channel_req *rq)
 	struct bchannel	*bch;
 	int		ch;
 
+	if (rq->adr.channel > MISDN_MAX_CHANNEL)
+		return -EINVAL;
 	if (!test_bit(rq->adr.channel & 0x1f,
-		&dch->dev.channelmap[rq->adr.channel >> 5]))
+		(u_long *)&dch->dev.channelmap[rq->adr.channel >> 5]))
 		return -EINVAL;
 	if (rq->protocol == ISDN_P_NONE)
 		return -EINVAL;
diff --git a/drivers/isdn/mISDN/socket.c b/drivers/isdn/mISDN/socket.c
index 4ba4cc3..e5a20f9 100644
--- a/drivers/isdn/mISDN/socket.c
+++ b/drivers/isdn/mISDN/socket.c
@@ -379,7 +379,7 @@ data_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 			di.Bprotocols = dev->Bprotocols | get_all_Bprotocols();
 			di.protocol = dev->D.protocol;
 			memcpy(di.channelmap, dev->channelmap,
-				MISDN_CHMAP_SIZE * 4);
+				sizeof(di.channelmap));
 			di.nrbchan = dev->nrbchan;
 			strcpy(di.name, dev->name);
 			if (copy_to_user((void __user *)arg, &di, sizeof(di)))
@@ -637,7 +637,7 @@ base_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 			di.Bprotocols = dev->Bprotocols | get_all_Bprotocols();
 			di.protocol = dev->D.protocol;
 			memcpy(di.channelmap, dev->channelmap,
-				MISDN_CHMAP_SIZE * 4);
+				sizeof(di.channelmap));
 			di.nrbchan = dev->nrbchan;
 			strcpy(di.name, dev->name);
 			if (copy_to_user((void __user *)arg, &di, sizeof(di)))
diff --git a/include/linux/mISDNif.h b/include/linux/mISDNif.h
index 5c948f3..0172ea8 100644
--- a/include/linux/mISDNif.h
+++ b/include/linux/mISDNif.h
@@ -37,7 +37,7 @@
  */
 #define	MISDN_MAJOR_VERSION	1
 #define	MISDN_MINOR_VERSION	0
-#define MISDN_RELEASE		18
+#define MISDN_RELEASE		19
 
 /* primitives for information exchange
  * generell format
@@ -242,7 +242,8 @@ struct mISDNhead {
 #define TEI_SAPI		63
 #define CTRL_SAPI		0
 
-#define MISDN_CHMAP_SIZE	4
+#define MISDN_MAX_CHANNEL	127
+#define MISDN_CHMAP_SIZE	((MISDN_MAX_CHANNEL + 1) >> 5)
 
 #define SOL_MISDN	0
 
@@ -275,7 +276,7 @@ struct mISDN_devinfo {
 	u_int			Dprotocols;
 	u_int			Bprotocols;
 	u_int			protocol;
-	u_long			channelmap[MISDN_CHMAP_SIZE];
+	u_int			channelmap[MISDN_CHMAP_SIZE];
 	u_int			nrbchan;
 	char			name[MISDN_MAX_IDLEN];
 };
-- 
1.5.6.4


-- 
Karsten Keil
SuSE Labs
ISDN and VOIP development
SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg)

Re: [PATCH] mISDN cleanup user interface

[David Woodhouse]

On Tue, 2008-07-29 at 21:56 +0200, Karsten Keil wrote:
> Please pull from git://git.kernel.org/pub/scm/linux/kernel/git/kkeil/ISDN-2.6.git master
> 
> The channelmap should have the same size on 32 and 64 bit
> systems. Thanks to David Woodhouse for spotting this.
> 
> Signed-off-by: Karsten Keil <kkeil@suse.de>

Er, isn't that still broken? Consider 32-bit userspace on 64-bit kernel,
big-endian.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] mISDN cleanup user interface

[Marcel Holtmann]

Hi Karsten,

> > Please pull from git://git.kernel.org/pub/scm/linux/kernel/git/kkeil/ISDN-2.6.git master
> > 
> > The channelmap should have the same size on 32 and 64 bit
> > systems. Thanks to David Woodhouse for spotting this.
> > 
> > Signed-off-by: Karsten Keil <kkeil@suse.de>
> 
> Er, isn't that still broken? Consider 32-bit userspace on 64-bit kernel,
> big-endian.

I agree with David here. Please lets not do these horribly things again.
Almost everybody has done their fair share of brokenness with the compat
layers. Especially 32-bit userspace on 64-bit kernels. Using __u32 and
alike is a way better choice and less headache.

Regards

Marcel

Re: [PATCH] mISDN cleanup user interface

[David Woodhouse]

On Tue, 2008-07-29 at 23:02 +0200, Marcel Holtmann wrote:
> Hi Karsten,
> 
> > > Please pull from git://git.kernel.org/pub/scm/linux/kernel/git/kkeil/ISDN-2.6.git master
> > > 
> > > The channelmap should have the same size on 32 and 64 bit
> > > systems. Thanks to David Woodhouse for spotting this.
> > > 
> > > Signed-off-by: Karsten Keil <kkeil@suse.de>
> > 
> > Er, isn't that still broken? Consider 32-bit userspace on 64-bit kernel,
> > big-endian.
> 
> I agree with David here. Please lets not do these horribly things again.
> Almost everybody has done their fair share of brokenness with the compat
> layers. Especially 32-bit userspace on 64-bit kernels. Using __u32 and
> alike is a way better choice and less headache.

Well, he's already switched to a 32-bit type, which is fine in that
respect -- at least the struct is the same _size_ on 32-bit and 64-bit
targets now. (OK, so inventing new types like 'u_int' instead of just
using the ones that the C language provides is a bit silly, but that's
the Linux norm so I wasn't complaining about that.)

But the channelmap field is still broken because it's using set_bit() on
int types, and they're defined to work on unsigned long. So in the
fairly common case where a BRI driver sets bits 1 and 2 (sic) in the
channel map, that will look like this on a 64-bit big-endian kernel:

 00 00 00 00 00 00 00 06   00 00 00 00 00 00 00 00
    (unsigned long #1)       (unsigned long #2)

When 32-bit userspace receives that, it'll see this:

 00 00 00 00   00 00 00 06   00 00 00 00   00 00 00 00
  (word #1)     (word #2)     (word #3)     (word #4)

.. in which it's bits 33 and 34 that are set. So the new version in the
patch to which I replied _still_ needs a compat routine.

You might get away with it if you use set_le_bit() though.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] mISDN cleanup user interface

[Karsten Keil]

On Wed, Jul 30, 2008 at 10:13:31AM +0100, David Woodhouse wrote:
> On Tue, 2008-07-29 at 23:02 +0200, Marcel Holtmann wrote:
> > Hi Karsten,
> > 
> > > > Please pull from git://git.kernel.org/pub/scm/linux/kernel/git/kkeil/ISDN-2.6.git master
> > > > 
> > > > The channelmap should have the same size on 32 and 64 bit
> > > > systems. Thanks to David Woodhouse for spotting this.
> > > > 
> > > > Signed-off-by: Karsten Keil <kkeil@suse.de>
> > > 
> > > Er, isn't that still broken? Consider 32-bit userspace on 64-bit kernel,
> > > big-endian.
> > 

What about this implementation ?

>From a0969b4fd0bc88e155f3723bbc306a7b399c6112 Mon Sep 17 00:00:00 2001
From: Karsten Keil <kkeil@suse.de>
Date: Wed, 30 Jul 2008 18:26:58 +0200
Subject: [PATCH] mISDN cleanup user interface

The channelmap should have the same size on 32 and 64 bit systems
and should not depend on endianess.
Thanks to David Woodhouse for spotting this.

Signed-off-by: Karsten Keil <kkeil@suse.de>
---
 drivers/isdn/hardware/mISDN/hfcmulti.c |    6 +++---
 drivers/isdn/hardware/mISDN/hfcpci.c   |    2 +-
 drivers/isdn/mISDN/l1oip_core.c        |    6 ++----
 drivers/isdn/mISDN/socket.c            |    4 ++--
 include/linux/mISDNif.h                |   32 +++++++++++++++++++++++++++-----
 5 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/drivers/isdn/hardware/mISDN/hfcmulti.c b/drivers/isdn/hardware/mISDN/hfcmulti.c
index 2649ea5..10144e8 100644
--- a/drivers/isdn/hardware/mISDN/hfcmulti.c
+++ b/drivers/isdn/hardware/mISDN/hfcmulti.c
@@ -3971,7 +3971,7 @@ open_bchannel(struct hfc_multi *hc, struct dchannel *dch,
 	struct bchannel	*bch;
 	int		ch;
 
-	if (!test_bit(rq->adr.channel, &dch->dev.channelmap[0]))
+	if (!test_channelmap(rq->adr.channel, dch->dev.channelmap))
 		return -EINVAL;
 	if (rq->protocol == ISDN_P_NONE)
 		return -EINVAL;
@@ -4587,7 +4587,7 @@ init_e1_port(struct hfc_multi *hc, struct hm_map *m)
 		list_add(&bch->ch.list, &dch->dev.bchannels);
 		hc->chan[ch].bch = bch;
 		hc->chan[ch].port = 0;
-		test_and_set_bit(bch->nr, &dch->dev.channelmap[0]);
+		set_channelmap(bch->nr, dch->dev.channelmap);
 	}
 	/* set optical line type */
 	if (port[Port_cnt] & 0x001) {
@@ -4755,7 +4755,7 @@ init_multi_port(struct hfc_multi *hc, int pt)
 		list_add(&bch->ch.list, &dch->dev.bchannels);
 		hc->chan[i + ch].bch = bch;
 		hc->chan[i + ch].port = pt;
-		test_and_set_bit(bch->nr, &dch->dev.channelmap[0]);
+		set_channelmap(bch->nr, dch->dev.channelmap);
 	}
 	/* set master clock */
 	if (port[Port_cnt] & 0x001) {
diff --git a/drivers/isdn/hardware/mISDN/hfcpci.c b/drivers/isdn/hardware/mISDN/hfcpci.c
index 3231814..9cf5edb 100644
--- a/drivers/isdn/hardware/mISDN/hfcpci.c
+++ b/drivers/isdn/hardware/mISDN/hfcpci.c
@@ -2056,7 +2056,7 @@ setup_card(struct hfc_pci *card)
 	card->dch.dev.nrbchan = 2;
 	for (i = 0; i < 2; i++) {
 		card->bch[i].nr = i + 1;
-		test_and_set_bit(i + 1, &card->dch.dev.channelmap[0]);
+		set_channelmap(i + 1, card->dch.dev.channelmap);
 		card->bch[i].debug = debug;
 		mISDN_initbchannel(&card->bch[i], MAX_DATA_MEM);
 		card->bch[i].hw = card;
diff --git a/drivers/isdn/mISDN/l1oip_core.c b/drivers/isdn/mISDN/l1oip_core.c
index 155b997..e42150a 100644
--- a/drivers/isdn/mISDN/l1oip_core.c
+++ b/drivers/isdn/mISDN/l1oip_core.c
@@ -1006,8 +1006,7 @@ open_bchannel(struct l1oip *hc, struct dchannel *dch, struct channel_req *rq)
 	struct bchannel	*bch;
 	int		ch;
 
-	if (!test_bit(rq->adr.channel & 0x1f,
-		&dch->dev.channelmap[rq->adr.channel >> 5]))
+	if (!test_channelmap(rq->adr.channel, dch->dev.channelmap))
 		return -EINVAL;
 	if (rq->protocol == ISDN_P_NONE)
 		return -EINVAL;
@@ -1412,8 +1411,7 @@ init_card(struct l1oip *hc, int pri, int bundle)
 		bch->ch.nr = i + ch;
 		list_add(&bch->ch.list, &dch->dev.bchannels);
 		hc->chan[i + ch].bch = bch;
-		test_and_set_bit(bch->nr & 0x1f,
-			&dch->dev.channelmap[bch->nr >> 5]);
+		set_channelmap(bch->nr, dch->dev.channelmap);
 	}
 	ret = mISDN_register_device(&dch->dev, hc->name);
 	if (ret)
diff --git a/drivers/isdn/mISDN/socket.c b/drivers/isdn/mISDN/socket.c
index 4ba4cc3..e5a20f9 100644
--- a/drivers/isdn/mISDN/socket.c
+++ b/drivers/isdn/mISDN/socket.c
@@ -379,7 +379,7 @@ data_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 			di.Bprotocols = dev->Bprotocols | get_all_Bprotocols();
 			di.protocol = dev->D.protocol;
 			memcpy(di.channelmap, dev->channelmap,
-				MISDN_CHMAP_SIZE * 4);
+				sizeof(di.channelmap));
 			di.nrbchan = dev->nrbchan;
 			strcpy(di.name, dev->name);
 			if (copy_to_user((void __user *)arg, &di, sizeof(di)))
@@ -637,7 +637,7 @@ base_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 			di.Bprotocols = dev->Bprotocols | get_all_Bprotocols();
 			di.protocol = dev->D.protocol;
 			memcpy(di.channelmap, dev->channelmap,
-				MISDN_CHMAP_SIZE * 4);
+				sizeof(di.channelmap));
 			di.nrbchan = dev->nrbchan;
 			strcpy(di.name, dev->name);
 			if (copy_to_user((void __user *)arg, &di, sizeof(di)))
diff --git a/include/linux/mISDNif.h b/include/linux/mISDNif.h
index 5c948f3..8f2d60d 100644
--- a/include/linux/mISDNif.h
+++ b/include/linux/mISDNif.h
@@ -37,7 +37,7 @@
  */
 #define	MISDN_MAJOR_VERSION	1
 #define	MISDN_MINOR_VERSION	0
-#define MISDN_RELEASE		18
+#define MISDN_RELEASE		19
 
 /* primitives for information exchange
  * generell format
@@ -242,7 +242,8 @@ struct mISDNhead {
 #define TEI_SAPI		63
 #define CTRL_SAPI		0
 
-#define MISDN_CHMAP_SIZE	4
+#define MISDN_MAX_CHANNEL	127
+#define MISDN_CHMAP_SIZE	((MISDN_MAX_CHANNEL + 1) >> 3)
 
 #define SOL_MISDN	0
 
@@ -275,11 +276,32 @@ struct mISDN_devinfo {
 	u_int			Dprotocols;
 	u_int			Bprotocols;
 	u_int			protocol;
-	u_long			channelmap[MISDN_CHMAP_SIZE];
+	u_char			channelmap[MISDN_CHMAP_SIZE];
 	u_int			nrbchan;
 	char			name[MISDN_MAX_IDLEN];
 };
 
+static inline int
+test_channelmap(u_int nr, u_char *map)
+{
+	if (nr <= MISDN_MAX_CHANNEL)
+		return map[nr >> 3] & (1 << (nr & 7));
+	else
+		return 0;
+}
+
+static inline void
+set_channelmap(u_int nr, u_char *map)
+{
+	map[nr >> 3] |= (1 << (nr & 7));
+}
+
+static inline void
+clear_channelmap(u_int nr, u_char *map)
+{
+	map[nr >> 3] &= ~(1 << (nr & 7));
+}
+
 /* CONTROL_CHANNEL parameters */
 #define MISDN_CTRL_GETOP		0x0000
 #define MISDN_CTRL_LOOP			0x0001
@@ -405,7 +427,7 @@ struct mISDNdevice {
 	u_int			Dprotocols;
 	u_int			Bprotocols;
 	u_int			nrbchan;
-	u_long			channelmap[MISDN_CHMAP_SIZE];
+	u_char			channelmap[MISDN_CHMAP_SIZE];
 	struct list_head	bchannels;
 	struct mISDNchannel	*teimgr;
 	struct device		dev;
@@ -430,7 +452,7 @@ struct mISDNstack {
 #endif
 };
 
-/* global alloc/queue dunctions */
+/* global alloc/queue functions */
 
 static inline struct sk_buff *
 mI_alloc_skb(unsigned int len, gfp_t gfp_mask)
-- 
1.5.6.4



-- 
Karsten Keil
SuSE Labs
ISDN and VOIP development
SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg)

Re: [PATCH] mISDN cleanup user interface

[David Woodhouse]

On Wed, 2008-07-30 at 18:33 +0200, Karsten Keil wrote:
> What about this implementation ?

Looks a lot saner... although it does seem to confirm my earlier
suspicion that you're not even _using_ the fact that it's a bitmap at
all. You set the bits for the present channels at init time, which are
always contiguous, and you never seem to change them them later -- why
couldn't you do this with a simple 'number of channels' integer?

Are you later intending to use the bitmap to mark them as busy/free?

-- 
dwmw2

Re: [PATCH] mISDN cleanup user interface

[Karsten Keil]

On Wed, Jul 30, 2008 at 05:57:01PM +0100, David Woodhouse wrote:
> On Wed, 2008-07-30 at 18:33 +0200, Karsten Keil wrote:
> > What about this implementation ?
> 
> Looks a lot saner... although it does seem to confirm my earlier
> suspicion that you're not even _using_ the fact that it's a bitmap at
> all. You set the bits for the present channels at init time, which are
> always contiguous, and you never seem to change them them later -- why
> couldn't you do this with a simple 'number of channels' integer?

No it is not contineous on different PRI line setups (E1,T1 ...)
e.g a E1 has the D-channel on channel 15 position, so this bit is not set
then. My idea was, that with such a bitmap, applictions do not need to know
anything about the different channel layouts, it can use the map as base to assign
or validate a channel numbers.

> 
> Are you later intending to use the bitmap to mark them as busy/free?
> 

Yes exactely, and this was the reason why the original code (which used one u_long
only as channelmap) already used the bit operators, since for a channel allocator
you should be atomic, but since we are now allow 127 channels we need proper
locking for the busy/free map anyways and so we do not need atomic operation
here.

-- 
Karsten Keil
SuSE Labs
ISDN and VOIP development
SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg)

Re: [PATCH] mISDN cleanup user interface

[Sam Ravnborg]

On Wed, Jul 30, 2008 at 07:51:38PM +0200, Karsten Keil wrote:
> On Wed, Jul 30, 2008 at 05:57:01PM +0100, David Woodhouse wrote:
> > On Wed, 2008-07-30 at 18:33 +0200, Karsten Keil wrote:
> > > What about this implementation ?
> > 
> > Looks a lot saner... although it does seem to confirm my earlier
> > suspicion that you're not even _using_ the fact that it's a bitmap at
> > all. You set the bits for the present channels at init time, which are
> > always contiguous, and you never seem to change them them later -- why
> > couldn't you do this with a simple 'number of channels' integer?
> 
> No it is not contineous on different PRI line setups (E1,T1 ...)
> e.g a E1 has the D-channel on channel 15 position, so this bit is not set
> then.
timeslot 16 in my book actually - but maybe that your timeslot to
channel mapping.

Not that it has anything to do with the topic discussed!

	Sam

Re: [PATCH] mISDN cleanup user interface

[Karsten Keil]

On Wed, Jul 30, 2008 at 08:37:21PM +0200, Sam Ravnborg wrote:
> On Wed, Jul 30, 2008 at 07:51:38PM +0200, Karsten Keil wrote:
> > On Wed, Jul 30, 2008 at 05:57:01PM +0100, David Woodhouse wrote:
> > > On Wed, 2008-07-30 at 18:33 +0200, Karsten Keil wrote:
> > > > What about this implementation ?
> > > 
> > > Looks a lot saner... although it does seem to confirm my earlier
> > > suspicion that you're not even _using_ the fact that it's a bitmap at
> > > all. You set the bits for the present channels at init time, which are
> > > always contiguous, and you never seem to change them them later -- why
> > > couldn't you do this with a simple 'number of channels' integer?
> > 
> > No it is not contineous on different PRI line setups (E1,T1 ...)
> > e.g a E1 has the D-channel on channel 15 position, so this bit is not set
> > then.
> timeslot 16 in my book actually - but maybe that your timeslot to
> channel mapping.
> 

You are correct of course.

-- 
Karsten Keil
SuSE Labs
ISDN and VOIP development
SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg)

Re: [PATCH] mISDN cleanup user interface

[David Woodhouse]

On Wed, 2008-07-30 at 19:51 +0200, Karsten Keil wrote:
> > Are you later intending to use the bitmap to mark them as busy/free?
> 
> Yes exactely, and this was the reason why the original code (which used one u_long
> only as channelmap) already used the bit operators, since for a channel allocator
> you should be atomic, but since we are now allow 127 channels we need proper
> locking for the busy/free map anyways and so we do not need atomic operation
> here.

OK, that makes sense then. Your latest patch looks good in that case.

-- 
dwmw2

Re: [PATCH] mISDN cleanup user interface

[Karsten Keil]

On Tue, Jul 29, 2008 at 09:22:26PM +0100, David Woodhouse wrote:
> On Tue, 2008-07-29 at 21:56 +0200, Karsten Keil wrote:
> > Please pull from git://git.kernel.org/pub/scm/linux/kernel/git/kkeil/ISDN-2.6.git master
> > 
> > The channelmap should have the same size on 32 and 64 bit
> > systems. Thanks to David Woodhouse for spotting this.
> > 
> > Signed-off-by: Karsten Keil <kkeil@suse.de>
> 
> Er, isn't that still broken? Consider 32-bit userspace on 64-bit kernel,
> big-endian.
> 

URGH, you are right, using the bit operations here is not a good idea at
all and currently we also need not to be atomic for this map so we can use a
bytearray and simple operation here.

-- 
Karsten Keil
SuSE Labs
ISDN and VOIP development
SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg)

Re: [PATCH] mISDN cleanup user interface

[Linus Torvalds]

On Tue, 29 Jul 2008, Karsten Keil wrote:
>
> Please pull from git://git.kernel.org/pub/scm/linux/kernel/git/kkeil/ISDN-2.6.git master

Grr. What's the status of this now? That branch is unpullable - you've put 
pretty much the same patch in twice, reverted it once, and added some 
merge into it too. So of the five commits, less than half are actually 
useful and worthwhile.

		Linus

Re: [PATCH] mISDN cleanup user interface

[Karsten Keil]

On Fri, Aug 01, 2008 at 10:21:29AM -0700, Linus Torvalds wrote:
> 
> 
> On Tue, 29 Jul 2008, Karsten Keil wrote:
> >
> > Please pull from git://git.kernel.org/pub/scm/linux/kernel/git/kkeil/ISDN-2.6.git master
> 
> Grr. What's the status of this now? That branch is unpullable - you've put 
> pretty much the same patch in twice, reverted it once, and added some 
> merge into it too. So of the five commits, less than half are actually 
> useful and worthwhile.
> 

I created a new clean merge tree, it contain the last 4 patches I posted
some minutes ago.

git://git.kernel.org/pub/scm/linux/kernel/git/kkeil/ISDN-2.6.git master


-- 
Karsten Keil
SuSE Labs
ISDN and VOIP development
SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg)