The follow is the first baby step towards moving the HiSax drivers to
the new PCI API. The HiSax PCI initialization code is ass-backwards and
an incredible pain, so this will take many steps. If anybody is
motivated to assist, help is more than welcome!
The first step just moves some code into a separate function, with the
general direction being the isolation of card-generic code away from all
the heavily #ifdef'd card-specific code.
This patch should not change any behavior at all. It only includes very
trivial code modifications, like s/kmalloc/kzalloc/, use of gfp_mask,
and the allocation of cs->rcvbuf was moved up to be with the rest of the
memory allocations.
Further changes will be checked into this git branch:
The 'isdn-pci' branch of
git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git
contains the following updates:
drivers/isdn/hisax/config.c | 83 ++++++++++++++++++++++++++++--------------
1 files changed, 55 insertions(+), 28 deletions(-)
Jeff Garzik (1):
[ISDN] hisax: split cs alloc and init away from checkcard()
diff --git a/drivers/isdn/hisax/config.c b/drivers/isdn/hisax/config.c
index da4196f..6c9a336 100644
--- a/drivers/isdn/hisax/config.c
+++ b/drivers/isdn/hisax/config.c
@@ -847,13 +847,14 @@ static int init_card(struct IsdnCardState *cs)
return 3;
}
-static int checkcard(int cardnr, char *id, int *busy_flag, struct module *lockowner)
+static int hisax_new_cs(int cardnr, struct IsdnCard *card,
+ struct IsdnCardState **cs_out, int *busy_flag,
+ struct module *lockowner, gfp_t gfp_mask)
{
- int ret = 0;
- struct IsdnCard *card = cards + cardnr;
struct IsdnCardState *cs;
+ int rc = -ENOMEM;
- cs = kzalloc(sizeof(struct IsdnCardState), GFP_ATOMIC);
+ cs = kzalloc(sizeof(struct IsdnCardState), gfp_mask);
if (!cs) {
printk(KERN_WARNING
"HiSax: No memory for IsdnCardState(card %d)\n",
@@ -870,30 +871,32 @@ static int checkcard(int cardnr, char *id, int *busy_flag, struct module *lockow
cs->HW_Flags = 0;
cs->busy_flag = busy_flag;
cs->irq_flags = I4L_IRQ_FLAG;
-#if TEI_PER_CARD
- if (card->protocol == ISDN_PTYPE_NI1)
- test_and_set_bit(FLG_TWO_DCHAN, &cs->HW_Flags);
-#else
- test_and_set_bit(FLG_TWO_DCHAN, &cs->HW_Flags);
-#endif
+
cs->protocol = card->protocol;
if (card->typ <= 0 || card->typ > ISDN_CTYPE_COUNT) {
printk(KERN_WARNING
"HiSax: Card Type %d out of range\n", card->typ);
+ rc = -EINVAL;
goto outf_cs;
}
- if (!(cs->dlog = kmalloc(MAX_DLOG_SPACE, GFP_ATOMIC))) {
+
+ if (!(cs->dlog = kzalloc(MAX_DLOG_SPACE, gfp_mask))) {
printk(KERN_WARNING
"HiSax: No memory for dlog(card %d)\n", cardnr + 1);
goto outf_cs;
}
- if (!(cs->status_buf = kmalloc(HISAX_STATUS_BUFSIZE, GFP_ATOMIC))) {
+ if (!(cs->status_buf = kzalloc(HISAX_STATUS_BUFSIZE, gfp_mask))) {
printk(KERN_WARNING
"HiSax: No memory for status_buf(card %d)\n",
cardnr + 1);
goto outf_dlog;
}
+ if (!(cs->rcvbuf = kzalloc(MAX_DFRAME_LEN_L1, gfp_mask))) {
+ printk(KERN_WARNING "HiSax: No memory for isac rcvbuf\n");
+ goto outf_status;
+ }
+
cs->stlist = NULL;
cs->status_read = cs->status_buf;
cs->status_write = cs->status_buf;
@@ -911,22 +914,53 @@ static int checkcard(int cardnr, char *id, int *busy_flag, struct module *lockow
ISDN_FEATURE_L2_HDLC |
ISDN_FEATURE_L2_HDLC_56K |
ISDN_FEATURE_L2_TRANS |
- ISDN_FEATURE_L3_TRANS |
+ ISDN_FEATURE_L3_TRANS;
+ cs->iif.command = HiSax_command;
+ cs->iif.writecmd = NULL;
+ cs->iif.writebuf_skb = HiSax_writebuf_skb;
+ cs->iif.readstat = HiSax_readstatus;
+
+ *cs_out = cs;
+ return 0;
+
+outf_status:
+ kfree(cs->status_buf);
+outf_dlog:
+ kfree(cs->dlog);
+outf_cs:
+ kfree(cs);
+ card->cs = NULL;
+out:
+ *cs_out = NULL;
+ return rc;
+}
+
+static int checkcard(int cardnr, char *id, int *busy_flag, struct module *lockowner)
+{
+ int ret = 0, rc;
+ struct IsdnCard *card = cards + cardnr;
+ struct IsdnCardState *cs = NULL;
+
+ rc = hisax_new_cs(cardnr, card, &cs, busy_flag, lockowner, GFP_ATOMIC);
+ if (rc)
+ goto out; /* ret == 0 == error */
+
+#if TEI_PER_CARD
+ if (card->protocol == ISDN_PTYPE_NI1)
+ test_and_set_bit(FLG_TWO_DCHAN, &cs->HW_Flags);
+#else
+ test_and_set_bit(FLG_TWO_DCHAN, &cs->HW_Flags);
+#endif
#ifdef CONFIG_HISAX_1TR6
- ISDN_FEATURE_P_1TR6 |
+ cs->iif.features |= ISDN_FEATURE_P_1TR6;
#endif
#ifdef CONFIG_HISAX_EURO
- ISDN_FEATURE_P_EURO |
+ cs->iif.features |= ISDN_FEATURE_P_EURO;
#endif
#ifdef CONFIG_HISAX_NI1
- ISDN_FEATURE_P_NI1 |
+ cs->iif.features |= ISDN_FEATURE_P_NI1;
#endif
- 0;
- cs->iif.command = HiSax_command;
- cs->iif.writecmd = NULL;
- cs->iif.writebuf_skb = HiSax_writebuf_skb;
- cs->iif.readstat = HiSax_readstatus;
register_isdn(&cs->iif);
cs->myid = cs->iif.channels;
printk(KERN_INFO
@@ -1101,11 +1135,6 @@ static int checkcard(int cardnr, char *id, int *busy_flag, struct module *lockow
ll_unload(cs);
goto outf_cs;
}
- if (!(cs->rcvbuf = kmalloc(MAX_DFRAME_LEN_L1, GFP_ATOMIC))) {
- printk(KERN_WARNING "HiSax: No memory for isac rcvbuf\n");
- ll_unload(cs);
- goto outf_cs;
- }
cs->rcvidx = 0;
cs->tx_skb = NULL;
cs->tx_cnt = 0;
@@ -1146,8 +1175,6 @@ static int checkcard(int cardnr, char *id, int *busy_flag, struct module *lockow
ret = 1;
goto out;
- outf_dlog:
- kfree(cs->dlog);
outf_cs:
kfree(cs);
card->cs = NULL;
Hello Jeff,
thank you for this effort.
On Tue, May 22, 2007 at 08:44:58AM -0400, Jeff Garzik wrote:
>
> The follow is the first baby step towards moving the HiSax drivers to
> the new PCI API. The HiSax PCI initialization code is ass-backwards and
> an incredible pain, so this will take many steps. If anybody is
> motivated to assist, help is more than welcome!
>
So what is the reason to do this now, does this block some changes in
other places ? I did not change this code for long time because I know
that this is a nightmare, the code was grown and grown (supporting more and
more cards/chipsets) and is really nothing which should be used in future.
So I would say it maybe better, if it doesn't block anything else, to
replace HiSax completely with a new modular design. I know that here was not
so much effort in the past years, but now I got some time to push mISDN
forward and it looks not so bad to get the first parts ready for mainline
in June.
Neverless, if you still think that we should get rid of the old HiSax
code, I'll help as much as I can, I still have most of the cards available
in my lab, so I can also test all changes.
> The first step just moves some code into a separate function, with the
> general direction being the isolation of card-generic code away from all
> the heavily #ifdef'd card-specific code.
>
> This patch should not change any behavior at all. It only includes very
> trivial code modifications, like s/kmalloc/kzalloc/, use of gfp_mask,
> and the allocation of cs->rcvbuf was moved up to be with the rest of the
> memory allocations.
>
> Further changes will be checked into this git branch:
>
> The 'isdn-pci' branch of
> git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git
>
...
--
Karsten Keil
SuSE Labs
ISDN and VOIP development
SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg)
Karsten Keil wrote:
> So what is the reason to do this now, does this block some changes in
> other places ? I did not change this code for long time because I know
> that this is a nightmare, the code was grown and grown (supporting more and
> more cards/chipsets) and is really nothing which should be used in future.
> So I would say it maybe better, if it doesn't block anything else, to
> replace HiSax completely with a new modular design. I know that here was not
> so much effort in the past years, but now I got some time to push mISDN
> forward and it looks not so bad to get the first parts ready for mainline
> in June.
ISDN is the biggest chunk of code blocking removal of some deprecated
PCI interfaces.
If you want to replace the code completely with a newfangled modular
design, I will gladly step out of the way and let that happen :) I only
know what I see right now, which is code that has sat around for a while
not getting updated to the new PCI API.
Since I do not have any ISDN hardware, my plan was to do equivalent
transformations of the code. Each patch just shuffles code, but
maintains a working state at all time, making it trivial to prove
(mathematically, or with testing, or with git-bisect) that the code
continues to work.
So, just tell me which you would prefer. If you are going to fix all
this stuff, there is plenty of other stuff on my list to work on.
Otherwise I will create equivalent-transform patches that take the
existing code and modify it just enough to be correct for the new PCI API.
Jeff
On Tue, May 22, 2007 at 12:46:16PM -0400, Jeff Garzik wrote:
> Karsten Keil wrote:
> >So what is the reason to do this now, does this block some changes in
> >other places ? I did not change this code for long time because I know
> >that this is a nightmare, the code was grown and grown (supporting more and
> >more cards/chipsets) and is really nothing which should be used in future.
> >So I would say it maybe better, if it doesn't block anything else, to
> >replace HiSax completely with a new modular design. I know that here was
> >not
> >so much effort in the past years, but now I got some time to push mISDN
> >forward and it looks not so bad to get the first parts ready for mainline
> >in June.
>
> ISDN is the biggest chunk of code blocking removal of some deprecated
> PCI interfaces.
>
OK then this should be done of course, since I fear that we will not
move all supported cards (since they are not longer sold) to the new driver.
> If you want to replace the code completely with a newfangled modular
> design, I will gladly step out of the way and let that happen :) I only
> know what I see right now, which is code that has sat around for a while
> not getting updated to the new PCI API.
>
> Since I do not have any ISDN hardware, my plan was to do equivalent
> transformations of the code. Each patch just shuffles code, but
> maintains a working state at all time, making it trivial to prove
> (mathematically, or with testing, or with git-bisect) that the code
> continues to work.
So maybe it could be a good start point to do the transition for one
card first, maybe nj_s.c is OK to start.
>
> So, just tell me which you would prefer. If you are going to fix all
> this stuff, there is plenty of other stuff on my list to work on.
> Otherwise I will create equivalent-transform patches that take the
> existing code and modify it just enough to be correct for the new PCI API.
>
If you do that for one card, I will test it and change the rest.
Thanks for your help.
--
Karsten Keil
SuSE Labs
ISDN and VOIP development
SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg)