[speedtouch] Re: [CVS commits] Various small things
Hi Edouard, Tim, > Tim Woodall ([EMAIL PROTECTED]) wrote: > > Wouldn't > > num_bytes_read -= nb_cells * ATM_CELL_TOTAL_SIZE; > > num_bytes_new = num_bytes_read; > > be better? > > Yes it would be better, so much better that i've never commited > something else than this piece of code :-) I noticed this, and indeed the code is probably more obvious this way. The essence is that you don't have to calculate nb_cells * ATM_CELL_TOTAL_SIZE twice. The double assignment was just some showoff geekery ;-) . Bye, Leonard. -- How clean is a war when you shoot around nukelar waste? Stop the use of depleted uranium ammo! End all weapons of mass destruction. Liste de diffusion modem ALCATEL SpeedTouch USB Pour se désinscrire : mailto:[EMAIL PROTECTED]
[speedtouch] Re: [CVS commits] Various small things
Tim Woodall ([EMAIL PROTECTED]) wrote: > Wouldn't > num_bytes_read -= nb_cells * ATM_CELL_TOTAL_SIZE; > num_bytes_new = num_bytes_read; > be better? Yes it would be better, so much better that i've never commited something else than this piece of code :-) Patches aren't applied blindly. -- Edouard Gomez Liste de diffusion modem ALCATEL SpeedTouch USB Pour se désinscrire : mailto:[EMAIL PROTECTED]
[speedtouch] Re: [CVS commits] Various small things
On Thu, 20 Nov 2003, Leonard den Ottolander wrote: > /* calculate the rest if there is any */ > - num_bytes_new = num_bytes_read - > nb_cells*ATM_CELL_TOTAL_SIZE; > - num_bytes_read -= nb_cells*ATM_CELL_TOTAL_SIZE; > + num_bytes_new = num_bytes_read -= nb_cells * > ATM_CELL_TOTAL_SIZE; > It's a style thing but this I really don't like. It is far too easy to overlook the - in the -= in the changed version. IMnsHO, the original was better. Wouldn't num_bytes_read -= nb_cells * ATM_CELL_TOTAL_SIZE; num_bytes_new = num_bytes_read; be better? Tim. -- God said, "div D = rho, div B = 0, curl E = - @B/@t, curl H = J + @D/@t," and there was light. http://tjw.hn.org/ http://www.locofungus.btinternet.co.uk/ Liste de diffusion modem ALCATEL SpeedTouch USB Pour se désinscrire : mailto:[EMAIL PROTECTED]
[speedtouch] Re: [CVS commits] Various small things
Hello Edouard,
> The changes may show up in the anonymous cvs soon[1].
They finally did. I see in the changelog says "Checks for
frames that are too long.", but that part you actually dropped.
Against the current pppoa3.c the following minor cleanups
could be applied:
$ diff -pu pppoa3.c.000 pppoa3.c
--- pppoa3.c.000Thu Nov 20 13:57:12 2003
+++ pppoa3.cThu Nov 20 14:09:54 2003
@@ -1002,10 +1002,10 @@ static void *read_from_usb_thread(void*
int n;
int pti;
int num_bytes_new;
- unsigned char lbuf[64*53];
+ unsigned char lbuf[64 * ATM_CELL_TOTAL_SIZE];
unsigned char *unused_cells;
- /* Reads 64*53 bytes from usb */
+ /* Reads 64 * ATM_CELL_TOTAL_SIZE bytes from usb */
do {
n = pusb_endpoint_read(ep_data, lbuf, sizeof(lbuf), 0);
} while (n < 0 && (errno == EINTR || errno == ETIMEDOUT));
@@ -1071,8 +1071,7 @@ static void *read_from_usb_thread(void*
nb_cells = pos / ATM_CELL_DATA_SIZE;
/* calculate the rest if there is any */
- num_bytes_new = num_bytes_read -
nb_cells*ATM_CELL_TOTAL_SIZE;
- num_bytes_read -= nb_cells*ATM_CELL_TOTAL_SIZE;
+ num_bytes_new = num_bytes_read -= nb_cells *
ATM_CELL_TOTAL_SIZE;
/* Reset the frame position */
pos = 0;
--- end of patch ---
That about covers it. By the way, thanks for accepting the patch.
Bye,
Leonard.
--
How clean is a war when you shoot around nukelar waste?
Stop the use of depleted uranium ammo!
End all weapons of mass destruction.
Liste de diffusion modem ALCATEL SpeedTouch USB
Pour se désinscrire : mailto:[EMAIL PROTECTED]
[speedtouch] Re: [CVS commits] Various small things
Hello Edouard, > I guess the sourceforge anonymous cvs is still in a pity state and > lagging way behind the developer cvs access. > > This patch was already applied with the changes I listed. It was not a > patch modification request but a report. Let's blame sourceforge that > made you loose time on this. It's not that bad. Maybe you didn't change the declaration of lbuf to use ATM_CELL_TOTAL_SIZE yet, and there are other changes I mentioned that were not in the patch I submitted to CVS. > The changes may show up in the anonymous cvs soon[1]. Ok. Did you also apply the changes you mentioned yesterday (fe using parameters from atm.h)? Note that the merging of the two if (pti == 1) 's is probably still a good idea, as well as the combination of the nb_cells and num_bytes_new calculation. If you send me a g/bzipped tarball of the current state of CVS I will apply these changes to that. Or you'll have to wait until anonymous CVS gets updated and I can patch against that. If I didn't oversee anything that should wrap up this thread. Bye, Leonard. -- How clean is a war when you shoot around nukelar waste? Stop the use of depleted uranium ammo! End all weapons of mass destruction. Liste de diffusion modem ALCATEL SpeedTouch USB Pour se désinscrire : mailto:[EMAIL PROTECTED]
[speedtouch] Re: [CVS commits] Various small things
Hi, (One man thread :) > Correction. unused_cells is initialised to lbuf at the first loop, and the > value of unused_cells is changed to atm_pointer by the call to > aal5_frame_from_atm_cells. This is probably a way to walk through lbuf. Ie > the use of unused_cells instead of lbuf makes sense. I'll have to look at > this a little closer, but I guess it is correct. > > The lazy mans way to check if it behaves correctly is by using a small > lbuf. Guess I'll have a go at that ;) . Using an lbuf size of 4 * 53 works just fine, so I assume the logic is correct. I do see a "kernel: usb-uhci.c: interrupt, status 3, frame# 123" every few seconds. Probably some congestion due to the excessive amount of calls to pusb_endpoint_read when using such a small buffer. I think the patch I just submitted can not be cleaned up any further, except maybe some stripping of the comments. Bye, Leonard. -- How clean is a war when you shoot around nukelar waste? Stop the use of depleted uranium ammo! End all weapons of mass destruction. Liste de diffusion modem ALCATEL SpeedTouch USB Pour se désinscrire : mailto:[EMAIL PROTECTED]
[speedtouch] Re: [CVS commits] Various small things
Leonard den Ottolander ([EMAIL PROTECTED]) wrote: > The two checks whether pti == 1 can be merged. Dropping the "idiotic > frame length" check, using the constants from atm.h, renaming n1 to > nb_cells, moving the declaration of nb_cells inside the if (pti == 1) > and simplifiying the payload length calculation renders the following > patch: I guess the sourceforge anonymous cvs is still in a pity state and lagging way behind the developer cvs access. This patch was already applied with the changes I listed. It was not a patch modification request but a report. Let's blame sourceforge that made you loose time on this. The changes may show up in the anonymous cvs soon[1]. [1] soon, being a notion that can vary a lot lately on sourceforge, 24h hours or even a week in worst case. -- Edouard Gomez Liste de diffusion modem ALCATEL SpeedTouch USB Pour se désinscrire : mailto:[EMAIL PROTECTED]
[speedtouch] Re: [CVS commits] Various small things
Hi,
I wrote:
> > The lbuf usage is still needed, it's the reading buffer. unused_cell is
> > just a pointer that keeps tracks of pppoa3 progress into the read buffer
> > if there are still ATM cells left in.
>
> Then that should be changed back. The current patch doesn't seem to use
> lbuf at all.
Correction. unused_cells is initialised to lbuf at the first loop, and the
value of unused_cells is changed to atm_pointer by the call to
aal5_frame_from_atm_cells. This is probably a way to walk through lbuf. Ie
the use of unused_cells instead of lbuf makes sense. I'll have to look at this
a little closer, but I guess it is correct.
The lazy mans way to check if it behaves correctly is by using a small lbuf. Guess
I'll have a go at that ;) .
> > I have just a doubt about the case where:
> > - num_bytes_read < 0
> >
> > It seems to me, that this case should be checked and invalidate the
> > unused_cell data.
>
> Isn't unused_cells set to NULL by aal5_frame_from_atm_cell() after the
> whole frame has been processed?
Indeed unused_cells is set to NULL. Also I don't really get your point.
Whenever num_bytes_read is not zero -num_bytes_read will be smaller than 0...
The two checks whether pti == 1 can be merged. Dropping the "idiotic frame
length" check, using the constants from atm.h, renaming n1 to nb_cells,
moving the declaration of nb_cells inside the if (pti == 1) and simplifiying
the payload length calculation renders the following patch:
diff -pruN speedtouch.000/src/pppoa3.c speedtouch/src/pppoa3.c
--- speedtouch.000/src/pppoa3.c Tue Nov 18 10:39:35 2003
+++ speedtouch/src/pppoa3.c Tue Nov 18 12:13:08 2003
@@ -955,7 +955,8 @@ static void *read_from_usb_thread(void*
{
sigset_t signal_set;
- unsigned int pos;
+ int pos;
+ int num_bytes_read = 0;
unsigned char *buffer;
unsigned char *aal5_recv_buf;
unsigned char *destination_buf;
@@ -1002,10 +1003,11 @@ static void *read_from_usb_thread(void*
int n;
int pti;
- unsigned char lbuf[64*53];
+ int num_bytes_new;
+ unsigned char lbuf[64 * ATM_CELL_TOTAL_SIZE];
unsigned char *unused_cells;
- /* Reads 64*53 bytes from usb */
+ /* Reads 64 * ATM_CELL_TOTAL_SIZE bytes from usb */
do {
n = pusb_endpoint_read(ep_data, lbuf, sizeof(lbuf), 0);
} while (n < 0 && (errno == EINTR || errno == ETIMEDOUT));
@@ -1018,55 +1020,62 @@ static void *read_from_usb_thread(void*
/* Debug information */
report(2, REPORT_DEBUG|REPORT_DATE|REPORT_DUMP, "ATM cells read from
USB (%d bytes long)\n", lbuf, n, n);
- /* Accumulates data in the aal5_recv buffer */
- /* pti will be equal to the last cell pti */
- pti = aal5_frame_from_atm_cells(aal5_recv_buf, lbuf, n, my_vpi,
my_vci, &pos, &unused_cells);
-
- /* A buffer overflow has been detected */
- if(pti<0) {
- report(0, REPORT_ERROR|REPORT_DATE, "Buffer overflow, too many
cells for the same aal5 frame\n");
- pti = 0;
- }
+ num_bytes_read += n; /* save total number of bytes to be
processed */
+ num_bytes_new = n;
+ unused_cells = lbuf; /* point at start of lbuf */
+
+ /* Accumulate all cell-data in the aal5_recv buffer */
+ /* Depending on how many cells and what type, we have to loop one or
more times until everything */
+ /* has been dealt with. (for example we read:
cell-cell-cell-'end'cell-cell-cell-cell*/
+ /* Every call to aal5_etc stops after finding an 'end'cell and then
pti = 1 ) 1 cell = 53 bytes */
+ while (unused_cells != NULL) {
+ pti = aal5_frame_from_atm_cells(aal5_recv_buf, unused_cells,
num_bytes_new, my_vpi, my_vci, &pos, &unused_cells);
+
+ /* 'pos' saves the place, where we are in the aal5_recv_buf */
+
+ /* A buffer overflow has been detected */
+ if (pti < 0) {
+ report(0, REPORT_ERROR|REPORT_DATE, "Buffer overflow,
too many cells for the same aal5 frame\n");
+ pti = 0;
+ }
- /* As the last pti is 1, we have to send the aal5_frame data */
- while(pti) {
+ /* pti = 0 (more frames to follow) or pti = 1 (end of
frame-group) */
+ /* When pti is 1, we have to send the aal5_frame data */
- /* Debug information */
- report(2, REPORT_DEBUG|REPORT_DATE|REPORT_DUMP, "AAL5 frame
joined up (%d bytes long)\n", aal5_recv_buf, pos, pos);
+ if (pti == 1) {
-
[speedtouch] Re: [CVS commits] Various small things
Hello Edouard, Just a quick reply, more tomorrow. > I just dropped the test. See if it makes CRC problems appear again. Just dropping the test for pos > 1536 shouldn't change anything afaict. Remember I inserted the else after that check. Also I always saw both "Idiotic frame length"s and CRC errors at the same time (before I inserted the else). That would suggest that dropping the first test shouldn't make a difference. > The lbuf usage is still needed, it's the reading buffer. unused_cell is > just a pointer that keeps tracks of pppoa3 progress into the read buffer if > there are still ATM cells left in. Then that should be changed back. The current patch doesn't seem to use lbuf at all. > I have just a doubt about the case where: > - num_bytes_read < 0 > > It seems to me, that this case should be checked and invalidate the > unused_cell data. Isn't unused_cells set to NULL by aal5_frame_from_atm_cell() after the whole frame has been processed? I did not check that. Again, I only submitted this patch because it addresses the CRC issue, I am not stating that it is entirely correct yet. This probably needs a little discussion (please refer to Ok Overbeek as well) and shouldn't be pushed to beta3 in 24 hours. > Can you check if a test like this one hurst the CRC solving: > if (num_bytes_read<0) unused_cells = NULL; > after the pos = 0; statement. I'll look at this tomorrow. Plus see above. By the way, num_bytes_read is the absolute number of bytes read so it will never be less than 0. And again, I assume unused_cells is set to NULL by aal5_frame_from_atm_cell(). Bye, Leonard. -- How clean is a war when you shoot around nukelar waste? Stop the use of depleted uranium ammo! End all weapons of mass destruction. Liste de diffusion modem ALCATEL SpeedTouch USB Pour se désinscrire : mailto:[EMAIL PROTECTED]
[speedtouch] Re: [CVS commits] Various small things
Leonard den Ottolander ([EMAIL PROTECTED]) wrote: > Is there a way to check the frame length against the negotiated length, or > do you just drop this check? No, pppoa daemons are just sort of bridging pipe. It's just a dumb piece of software: read data on one side, copy it to the other side. I just dropped the test. See if it makes CRC problems appear again. > > - some cleanup concerning the effective payload used > > The way aal5_frame_from_atm_cells() is called probably needs checking. I > am not sure if the use of unused_cells instead of lbuf is correct. Might > just work for small frame sizes. Also it seems that currently lbuf is not > used at all so if the use of unused_cells instead of lbuf is correct you > might just drop all references to lbuf. The lbuf usage is still needed, it's the reading buffer. unused_cell is just a pointer that keeps tracks of pppoa3 progress into the read buffer if there are still ATM cells left in. I have just a doubt about the case where: - num_bytes_read < 0 It seems to me, that this case should be checked and invalidate the unused_cell data. > Once again, this patch was not written by me, I just did some cleanup > and separation of functionality. I did not verify all the details of > its inner working, but can verify that it fixes the CRC error problem > I saw when using the KQD6P2.eni firmware with the unpatched > driver. But it definitely needs another check ;) . Can you check if a test like this one hurst the CRC solving: if (num_bytes_read<0) unused_cells = NULL; after the pos = 0; statement. -- Edouard Gomez Liste de diffusion modem ALCATEL SpeedTouch USB Pour se désinscrire : mailto:[EMAIL PROTECTED]
[speedtouch] Re: [CVS commits] Various small things
Hello Edouard, > Ok concerning the second patch, i'm going to apply it with slight > changes: > - the 'idiotic' message isn't good as this driver can be used on >networks with mtu > 1500. Not all the networks are Ethernet, and it's up >to your ISP to negotiate the MTU value through pppd or whatever. Is there a way to check the frame length against the negotiated length, or do you just drop this check? > - some cleanup concerning the effective payload used The way aal5_frame_from_atm_cells() is called probably needs checking. I am not sure if the use of unused_cells instead of lbuf is correct. Might just work for small frame sizes. Also it seems that currently lbuf is not used at all so if the use of unused_cells instead of lbuf is correct you might just drop all references to lbuf. Once again, this patch was not written by me, I just did some cleanup and separation of functionality. I did not verify all the details of its inner working, but can verify that it fixes the CRC error problem I saw when using the KQD6P2.eni firmware with the unpatched driver. But it definitely needs another check ;) . > -> use of constants defined in atm.h Sure. > and move n1 to the inner if(pti==1) >subcase. n1 has been renamed to nb_cells as well. And sure, sure again :-) . Bye, Leonard. -- How clean is a war when you shoot around nukelar waste? Stop the use of depleted uranium ammo! End all weapons of mass destruction. Liste de diffusion modem ALCATEL SpeedTouch USB Pour se désinscrire : mailto:[EMAIL PROTECTED]
[speedtouch] Re: [CVS commits] Various small things
Gilles Espinasse ([EMAIL PROTECTED]) wrote: > And the patch to modem_run send a few days ago by Howard Wilkinson? Sure. I just turned the -T option into -t to have lower cases option only (and -t was not used). And added some comments (2 iirc) and turned the last return statement surrounded by if, with a ()?:; operator (nicer ;-) Do you see anything else important i miss and that should be part of beta3 ? I give a 24h delay so people can complain, then i'll release beta3 if no major problem is found. 2003-11-17 23:14:56 GMT patch-13 Summary: Added timeout in modem_run. Revision: speedtouch--trunk--1.2--patch-13 Added timeout alarm to give up connecting in modem_run. modified files: ChangeLog doc-linux/man/modem_run.1 src/modem_run.c -- Edouard Gomez Liste de diffusion modem ALCATEL SpeedTouch USB Pour se désinscrire : mailto:[EMAIL PROTECTED]
[speedtouch] Re: [CVS commits] Various small things
And the patch to modem_run send a few days ago by Howard Wilkinson? Liste de diffusion modem ALCATEL SpeedTouch USB Pour se désinscrire : mailto:[EMAIL PROTECTED]
[speedtouch] Re: [CVS commits] Various small things
Leonard den Ottolander ([EMAIL PROTECTED]) wrote: > Please consider the patches I supplied via sourceforge before jumping > to beta3. TIA. Ok concerning the second patch, i'm going to apply it with slight changes: - the 'idiotic' message isn't good as this driver can be used on networks with mtu > 1500. Not all the networks are Ethernet, and it's up to your ISP to negotiate the MTU value through pppd or whatever. - some cleanup concerning the effective payload used -> use of constants defined in atm.h and move n1 to the inner if(pti==1) subcase. n1 has been renamed to nb_cells as well. -- Edouard Gomez Liste de diffusion modem ALCATEL SpeedTouch USB Pour se désinscrire : mailto:[EMAIL PROTECTED]
[speedtouch] Re: [CVS commits] Various small things
Leonard den Ottolander ([EMAIL PROTECTED]) wrote: > Please consider the patches I supplied via sourceforge before jumping > to beta3. TIA. Thanks to point me to them, as it's very rare to have patches on the sf site :-) The cleanup patch is not a problem, accepted as is. The second one will require a bit of review. I just need to check two or three things in the code i've just cross read. -- Edouard Gomez Liste de diffusion modem ALCATEL SpeedTouch USB Pour se désinscrire : mailto:[EMAIL PROTECTED]
[speedtouch] Re: [CVS commits] Various small things
Hello Edouard, > It's been a while since the last commits. This is probably going to be > released as beta3. Then i'll do rc releases for a final 1.2. Please consider the patches I supplied via sourceforge before jumping to beta3. TIA. Bye, Leonard. -- How clean is a war when you shoot around nukelar waste? Stop the use of depleted uranium ammo! End all weapons of mass destruction. Liste de diffusion modem ALCATEL SpeedTouch USB Pour se désinscrire : mailto:[EMAIL PROTECTED]
