| Message ID | 1335613404-10187-4-git-send-email-kkeil@linux-pingi.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Karsten Keil <kkeil@linux-pingi.de> Date: Sat, 28 Apr 2012 13:43:19 +0200 > The B-channels are only accessed via the PCM backplane. > Add infrastruckture for this special mode. > > Signed-off-by: Karsten Keil <kkeil@linux-pingi.de> I dread reviewing these ISDN patch sets because they are so full of problems, and it's so damn obvious how little care is put into preparing them. What I see is that you put the minimum amount of work necessary into splitting up your huge ISDN patch set submission into more managable pieces, and as a result you are introducing problems. > diff --git a/drivers/isdn/mISDN/socket.c b/drivers/isdn/mISDN/socket.c > index abe2d69..502bcf1 100644 > --- a/drivers/isdn/mISDN/socket.c > +++ b/drivers/isdn/mISDN/socket.c > @@ -270,6 +270,7 @@ data_sock_release(struct socket *sock) > break; > case ISDN_P_LAPD_TE: > case ISDN_P_LAPD_NT: > + case ISDN_P_B_PCM: > case ISDN_P_B_RAW: > case ISDN_P_B_HDLC: > case ISDN_P_B_X75SLP: Ok, that's fine. > @@ -148,6 +149,8 @@ struct bchannel { > u_int state; > void *hw; > int slot; /* multiport card channel slot */ > + int pcm_tx; /* PCM tx slot nr */ > + int pcm_rx; /* PCM rx slot nr */ > struct timer_list timer; > /* receive data */ > struct sk_buff *rx_skb; But what the hell is this? These structure members are unused by this patch, and in fact no patch in your entire series uses them. > @@ -360,8 +360,8 @@ clear_channelmap(u_int nr, u_char *map) > #define MISDN_CTRL_LOOP 0x0001 > #define MISDN_CTRL_CONNECT 0x0002 > #define MISDN_CTRL_DISCONNECT 0x0004 > -#define MISDN_CTRL_PCMCONNECT 0x0010 > -#define MISDN_CTRL_PCMDISCONNECT 0x0020 > +#define MISDN_CTRL_GET_PCM_SLOTS 0x0010 > +#define MISDN_CTRL_SET_PCM_SLOTS 0x0020 > #define MISDN_CTRL_SETPEER 0x0040 > #define MISDN_CTRL_UNSETPEER 0x0080 > #define MISDN_CTRL_RX_OFF 0x0100 Another completely unrelated change, nothing in this patch uses these new defines. > @@ -381,6 +381,10 @@ clear_channelmap(u_int nr, u_char *map) > #define MISDN_CTRL_HFC_WD_INIT 0x4009 > #define MISDN_CTRL_HFC_WD_RESET 0x400A > > +/* special PCM slot numbers */ > +#define MISDN_PCM_SLOT_DISABLE -1 /* PCM disabled */ > +#define MISDN_PCM_SLOT_IGNORE -2 /* PCM setting will be not changed */ > + > /* socket options */ > #define MISDN_TIME_STAMP 0x0001 > Same thing. > @@ -389,6 +393,7 @@ struct mISDN_ctrl_req { > int channel; > int p1; > int p2; > + int p3; > }; > > /* muxer options */ And again, same problem. You really need to get your act in gear and prepare your patches properly, so that they don't have unrelated changes in them. This is not amateur hour. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello David, Am 01.05.2012 19:30, schrieb David Miller: > From: Karsten Keil <kkeil@linux-pingi.de> > Date: Sat, 28 Apr 2012 13:43:19 +0200 > >> The B-channels are only accessed via the PCM backplane. >> Add infrastruckture for this special mode. >> >> Signed-off-by: Karsten Keil <kkeil@linux-pingi.de> > > I dread reviewing these ISDN patch sets because they are so > full of problems, and it's so damn obvious how little care is > put into preparing them. > > What I see is that you put the minimum amount of work necessary > into splitting up your huge ISDN patch set submission into more > managable pieces, and as a result you are introducing problems. > I did put the additional PCM infrastructure in this series, because the approval test was done with it in place. I did plan the update of the low level drivers in a separate patchset from the beginning. What do you prefer, adding the driver part now, as additional patch, or removing this additional infrastruckture part and submit it in a later series ? >> diff --git a/drivers/isdn/mISDN/socket.c b/drivers/isdn/mISDN/socket.c >> index abe2d69..502bcf1 100644 >> --- a/drivers/isdn/mISDN/socket.c >> +++ b/drivers/isdn/mISDN/socket.c >> @@ -270,6 +270,7 @@ data_sock_release(struct socket *sock) >> break; >> case ISDN_P_LAPD_TE: >> case ISDN_P_LAPD_NT: >> + case ISDN_P_B_PCM: >> case ISDN_P_B_RAW: >> case ISDN_P_B_HDLC: >> case ISDN_P_B_X75SLP: > > Ok, that's fine. > >> @@ -148,6 +149,8 @@ struct bchannel { >> u_int state; >> void *hw; >> int slot; /* multiport card channel slot */ >> + int pcm_tx; /* PCM tx slot nr */ >> + int pcm_rx; /* PCM rx slot nr */ >> struct timer_list timer; >> /* receive data */ >> struct sk_buff *rx_skb; > > But what the hell is this? These structure members are unused by > this patch, and in fact no patch in your entire series uses them. > >> @@ -360,8 +360,8 @@ clear_channelmap(u_int nr, u_char *map) >> #define MISDN_CTRL_LOOP 0x0001 >> #define MISDN_CTRL_CONNECT 0x0002 >> #define MISDN_CTRL_DISCONNECT 0x0004 >> -#define MISDN_CTRL_PCMCONNECT 0x0010 >> -#define MISDN_CTRL_PCMDISCONNECT 0x0020 >> +#define MISDN_CTRL_GET_PCM_SLOTS 0x0010 >> +#define MISDN_CTRL_SET_PCM_SLOTS 0x0020 >> #define MISDN_CTRL_SETPEER 0x0040 >> #define MISDN_CTRL_UNSETPEER 0x0080 >> #define MISDN_CTRL_RX_OFF 0x0100 > > Another completely unrelated change, nothing in this patch uses > these new defines. > >> @@ -381,6 +381,10 @@ clear_channelmap(u_int nr, u_char *map) >> #define MISDN_CTRL_HFC_WD_INIT 0x4009 >> #define MISDN_CTRL_HFC_WD_RESET 0x400A >> >> +/* special PCM slot numbers */ >> +#define MISDN_PCM_SLOT_DISABLE -1 /* PCM disabled */ >> +#define MISDN_PCM_SLOT_IGNORE -2 /* PCM setting will be not changed */ >> + >> /* socket options */ >> #define MISDN_TIME_STAMP 0x0001 >> > > Same thing. > >> @@ -389,6 +393,7 @@ struct mISDN_ctrl_req { >> int channel; >> int p1; >> int p2; >> + int p3; >> }; >> >> /* muxer options */ > > And again, same problem. > > You really need to get your act in gear and prepare your patches > properly, so that they don't have unrelated changes in them. > > This is not amateur hour. > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Karsten Keil <kkeil@linux-pingi.de> Date: Thu, 03 May 2012 08:31:57 +0200 > I did put the additional PCM infrastructure in this series, because > the approval test was done with it in place. > I did plan the update of the low level drivers in a separate patchset > from the beginning. > > What do you prefer, adding the driver part now, as additional patch, or > removing this additional infrastruckture part and submit it in a later > series ? I feel like I'm talking to a wall. A patch should do one, and only one thing. It should not have changes which are unrelated to that one thing. What part of this is so hard to understand? To make matters worse, you didn't even make a mention of those unrelated changes in your commit message. So that patch was bogus on at least two counts. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 03.05.2012 08:50, schrieb David Miller: > From: Karsten Keil <kkeil@linux-pingi.de> > Date: Thu, 03 May 2012 08:31:57 +0200 > >> I did put the additional PCM infrastructure in this series, because >> the approval test was done with it in place. >> I did plan the update of the low level drivers in a separate patchset >> from the beginning. >> >> What do you prefer, adding the driver part now, as additional patch, or >> removing this additional infrastruckture part and submit it in a later >> series ? > > I feel like I'm talking to a wall. > > A patch should do one, and only one thing. It should not have > changes which are unrelated to that one thing. > Sorry I disagree here, this patch does exactly one thing, it add the infrastructure in the mISDN core to allow the PCM only B-channel mode. PCM only mode need a special protocol and a mechanism to set/get/store the PCM slots of the card, and this is for what the extra stuff is used. > What part of this is so hard to understand? > > To make matters worse, you didn't even make a mention of those > unrelated changes in your commit message. > Yes, I was not verbose enough in the commit message and it was my error to include it in this series, without the use case. Maybe I'm too deep in the ISDN stuff so I forget that 3 parties do not see how it fit together. So would you accept this patch with a changed commit message like this ? Sometimes the ISDN chip only controls the D-channel The B-channels are connected only via the PCM backplane. For this special case we need two things, a protocol which do not enable the normal IO path and a method to set/get and store the PCM slots to be used on the backplane. This patch add the core infrastructure for both. A later patch will add the methods to the supported low level drivers. > So that patch was bogus on at least two counts. > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Karsten Keil <kkeil@linux-pingi.de> Date: Thu, 03 May 2012 09:31:04 +0200 > PCM only mode need a special protocol and a mechanism to set/get/store > the PCM slots of the card, and this is for what the extra stuff is used. It changed the values of some macros which are actually used by the code. Then it adds members to structures, and defines, which are completely unused. The latter part is completely bogus. This is the second time I'm saying this again. I'm not saying it a third time, instead I'll just ignore you. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/isdn/mISDN/socket.c b/drivers/isdn/mISDN/socket.c index abe2d69..502bcf1 100644 --- a/drivers/isdn/mISDN/socket.c +++ b/drivers/isdn/mISDN/socket.c @@ -270,6 +270,7 @@ data_sock_release(struct socket *sock) break; case ISDN_P_LAPD_TE: case ISDN_P_LAPD_NT: + case ISDN_P_B_PCM: case ISDN_P_B_RAW: case ISDN_P_B_HDLC: case ISDN_P_B_X75SLP: @@ -544,6 +545,7 @@ data_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_len) err = create_l2entity(_pms(sk)->dev, &_pms(sk)->ch, sk->sk_protocol, maddr); break; + case ISDN_P_B_PCM: case ISDN_P_B_RAW: case ISDN_P_B_HDLC: case ISDN_P_B_X75SLP: @@ -800,6 +802,7 @@ mISDN_sock_create(struct net *net, struct socket *sock, int proto, int kern) case ISDN_P_NT_E1: case ISDN_P_LAPD_TE: case ISDN_P_LAPD_NT: + case ISDN_P_B_PCM: case ISDN_P_B_RAW: case ISDN_P_B_HDLC: case ISDN_P_B_X75SLP: diff --git a/include/linux/mISDNhw.h b/include/linux/mISDNhw.h index 4af8414..74d5734 100644 --- a/include/linux/mISDNhw.h +++ b/include/linux/mISDNhw.h @@ -5,6 +5,7 @@ * Basic declarations for the mISDN HW channels * * Copyright 2008 by Karsten Keil <kkeil@novell.com> + * Copyright 2009-2012 by Karsten Keil <kkeil@linux-pingi.de> * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as @@ -148,6 +149,8 @@ struct bchannel { u_int state; void *hw; int slot; /* multiport card channel slot */ + int pcm_tx; /* PCM tx slot nr */ + int pcm_rx; /* PCM rx slot nr */ struct timer_list timer; /* receive data */ struct sk_buff *rx_skb; diff --git a/include/linux/mISDNif.h b/include/linux/mISDNif.h index b5e7f22..bdda647 100644 --- a/include/linux/mISDNif.h +++ b/include/linux/mISDNif.h @@ -37,7 +37,7 @@ */ #define MISDN_MAJOR_VERSION 1 #define MISDN_MINOR_VERSION 1 -#define MISDN_RELEASE 21 +#define MISDN_RELEASE 24 /* primitives for information exchange * generell format @@ -229,7 +229,7 @@ #define ISDN_P_B_MASK 0x1f #define ISDN_P_B_START 0x20 - +#define ISDN_P_B_PCM 0x20 #define ISDN_P_B_RAW 0x21 #define ISDN_P_B_HDLC 0x22 #define ISDN_P_B_X75SLP 0x23 @@ -360,8 +360,8 @@ clear_channelmap(u_int nr, u_char *map) #define MISDN_CTRL_LOOP 0x0001 #define MISDN_CTRL_CONNECT 0x0002 #define MISDN_CTRL_DISCONNECT 0x0004 -#define MISDN_CTRL_PCMCONNECT 0x0010 -#define MISDN_CTRL_PCMDISCONNECT 0x0020 +#define MISDN_CTRL_GET_PCM_SLOTS 0x0010 +#define MISDN_CTRL_SET_PCM_SLOTS 0x0020 #define MISDN_CTRL_SETPEER 0x0040 #define MISDN_CTRL_UNSETPEER 0x0080 #define MISDN_CTRL_RX_OFF 0x0100 @@ -381,6 +381,10 @@ clear_channelmap(u_int nr, u_char *map) #define MISDN_CTRL_HFC_WD_INIT 0x4009 #define MISDN_CTRL_HFC_WD_RESET 0x400A +/* special PCM slot numbers */ +#define MISDN_PCM_SLOT_DISABLE -1 /* PCM disabled */ +#define MISDN_PCM_SLOT_IGNORE -2 /* PCM setting will be not changed */ + /* socket options */ #define MISDN_TIME_STAMP 0x0001 @@ -389,6 +393,7 @@ struct mISDN_ctrl_req { int channel; int p1; int p2; + int p3; }; /* muxer options */
The B-channels are only accessed via the PCM backplane. Add infrastruckture for this special mode. Signed-off-by: Karsten Keil <kkeil@linux-pingi.de> --- drivers/isdn/mISDN/socket.c | 3 +++ include/linux/mISDNhw.h | 3 +++ include/linux/mISDNif.h | 13 +++++++++---- 3 files changed, 15 insertions(+), 4 deletions(-)