Re: [PATCH 1/5] Char: mxser, remove special baudrate processing

[Sergei Organov]

Jiri Slaby <[EMAIL PROTECTED]> writes:
> If you, Sergei, can test it, it would be great. [Note, that this is actually
> a patch for mxser_new after renaming.]

Where is the reference git tree for this patch? Or, in other words,
which tree do I use if I want to test this patch (I'm afraid the latest
I've ever built, 2.6.19.1, is too old for this material)?

I've also checked

git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git

and it seems it doesn't have commit that renames mxser_new to mxser.

-- 
Sergei.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/5] Char: mxser, remove special baudrate processing

[Sergei Organov]

Jiri Slaby [EMAIL PROTECTED] writes:
 If you, Sergei, can test it, it would be great. [Note, that this is actually
 a patch for mxser_new after renaming.]

Where is the reference git tree for this patch? Or, in other words,
which tree do I use if I want to test this patch (I'm afraid the latest
I've ever built, 2.6.19.1, is too old for this material)?

I've also checked

git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git

and it seems it doesn't have commit that renames mxser_new to mxser.

-- 
Sergei.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Gmail and flowed text

[Sergei Organov]

"Satyam Sharma" <[EMAIL PROTECTED]> writes:
[...]
> And yet another patch falls victim to format=flowed ...
>
> I need some ideas here myself:
>
> I don't want to subscribe from my university mail account -- I like
> to keep important messages on server, and the account has a
> 100 MB or so limit that wouldn't survive a week of lkml traffic.
>
> Which leaves Gmail,

You may also try free gmane service, , that is
mail-to-news gateway that has "gmane.linux.kernel" news group that is a
mirror of lkml.

-- 
Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Gmail and flowed text

[Sergei Organov]

Satyam Sharma [EMAIL PROTECTED] writes:
[...]
 And yet another patch falls victim to format=flowed ...

 I need some ideas here myself:

 I don't want to subscribe from my university mail account -- I like
 to keep important messages on server, and the account has a
 100 MB or so limit that wouldn't survive a week of lkml traffic.

 Which leaves Gmail,

You may also try free gmane service, http://www.gmane.org, that is
mail-to-news gateway that has gmane.linux.kernel news group that is a
mirror of lkml.

-- 
Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Linus Torvalds <[EMAIL PROTECTED]> writes:
> On Thu, 15 Feb 2007, Sergei Organov wrote:
>> 
>> I agree that if the warning has no true positives, it sucks. The problem
>> is that somehow I doubt it has none. And the reasons for the doubt are:
>
> Why do you harp on "no true positives"?

Because if somebody is capable to proof a warning has no true positives,
I immediately agree it's useless. I just wanted to check if it's indeed
the case. It seems it is not.

> That's a pointless thing. You can make *any* warning have "true 
> positives". My personal favorite is the unconditional warning:
>
>   warning: user is an idiot
>
> and I _guarantee_ you that it has a lot of true positives.

Yes, but there is obviously 0 correlation between the warning and the
user being an idiot (except that non-idiot will probably find a way to
either turn this warning off, or filter it out).

I already agreed that a warning may have true positives and still be
bad; and a warning having "no false negatives" is obviously a pretty
one, though unfortunately not that common in practice; and a warning
that has no true positives is useless. What we are arguing about are
intermediate cases that are most common in practice.

As programmers, we in fact are interested in correlation between a
warning and actual problem in the software, but for the kind of warnings
we are discussing (sign-correctness and type-safety in general), the
correlation depends on the style the program is written is, making it
difficult to use as a criteria in the discussion.

> It's the "no false negatives" angle you should look at.

I'd like to, but then I'll need to classify most of (all?) the GCC
warnings as bad, I'm afraid.

> THAT is what matters. The reason we don't see a lot of warnings about 
> idiot users is not that people don't do stupid things, but that 
> *sometimes* they actually don't do something stupid.
>
> Yeah, I know, it's far-fetched, but still.
>
> In other words, you're barking up *exactly* the wrong tree. You're looking 
> at it the wrong way around.
>
> Think of it this way: in science, a theory is proven to be bad by a single 
> undeniable fact just showing that it's wrong.
>
> The same is largely true of a warning. If the warning sometimes happens 
> for code that is perfectly fine, the warning is bad.

Consider:

int find_first_zero(const int *a)
{
int index = 0;
while(*a++ != 0)
index++;
return index;
}

unsigned int array[] = { 1, 2, 3, 0, 25, 14 };
int index = find_first_zero(array);  *WARNING*

So, by your logic, -Wpointer-sign warning is bad in general? If not,
then how will you "fix" the code above? And the upside of the fix is
what?

IMHO, the "problem" is that this warning is about violation of type
safety. Type safety by itself doesn't guarantee the code is perfectly
fine. Violation of type safety doesn't necessarily mean the code is
broken. A warning that warns about violation of type safety has the same
properties.  Now, it's up to you to decide if you want warnings on type
safety or not.

What you are saying, on the other hand, for me reads like this: "I do
want warnings on violations of type safety in general, except for
"signed char" or "unsigned char" being used instead of "char"". I think
I do understand your reasons, -- I just don't agree with them.

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Bodo Eggert <[EMAIL PROTECTED]> writes:
> On Fri, 16 Feb 2007, Sergei Organov wrote:

[...]

> I'll say it again: Either the code using unspecified chars is correct, or 
> it isn't. If it's correct, neither using with signed nor with unsigned 
> chars is a bug and you should not warn at all, and if it's not correct, 
> you should always warn. Instead, gcc warns on "code compiles for
> $arch".

Here is where we disagree. In my opinion, no matter what the sign of
char for given architecture is, it's incorrect to pass either "signed
char*" or "unsigned char*" argument to a function expecting "char*".

>> > Therefore it's either always wrong to call your char* function with char*,
>> > unsigned char* _and_ signed char unless you can guarantee not to overflow 
>> > any
>> > of them, or it's always correct to call char* functions with any kind
>> > of these.
>> 
>> How are you sure those who wrote foo(char*) agrees with your opinion or
>> even understands all the involved issues?
>
> Let's asume we have this piece of buggy code. We compile it on an unsigned
> char architecture. No warning. *BOOM*

There should be warning, -- that's my point. "char" is different. "char"
is distinct from either "signed char" or "unsigned char". Always. At
least it's how C is defined.

> Let's asume there is correct code, and we use it as designed:
> Warning: Wrong arch
> Warning: Wrong arch
> Warning: Wrong arch
> Warning: real issue 
> Warning: Wrong arch
> Warning: Wrong arch
> Warning: Wrong arch
> Warning: Wrong arch
> 
> Warning: Wrong arch
> Warning: Wrong arch
> Warning: Wrong arch
> Warning: Wrong arch
> Warning: Wrong arch
>
> You don't see "real issue". *BOOM*
>
>
> What can you do about this warning? Let's asume we cast everywhere:

I already gave an answer in response to Linus:

static inline size_t ustrlen(const unsigned char *s)
{
return strlen((const char *)s);
}

>
> struct foo * p;
> printf(strlen(char*)p); *BOOM*

printf(ustrlen(p)); *NO BOOM*
unsigned char* u;
printf(ustrlen(u)); *NO WARNING*

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Rene Herman <[EMAIL PROTECTED]> writes:
[...]
> Given char's special nature, shouldn't the conclusion of this thread
> have long been simply that gcc needs -Wno-char-pointer-sign? (with
> whatever default, as far as I'm concerned).

I entirely agree that all the char business in C is messy enough to
justify separate warning switch(es) in GCC.

However, I still insist that the problem with the code:

   void foo(char *c);
   unsigned char *u;
   signed char *s;
   ...
   foo(u);
   foo(s);

is not (only) in signedness, as neither 'u' nor 's' has compatible type
with the "char*", no matter what is the sign of "char", so if one cares
about type safety he needs warnings on both invocations of foo().

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Bodo Eggert <[EMAIL PROTECTED]> writes:
> On Fri, 16 Feb 2007, Sergei Organov wrote:
>> Bodo Eggert <[EMAIL PROTECTED]> writes:
>> > Sergei Organov <[EMAIL PROTECTED]> wrote:
>> >> Linus Torvalds <[EMAIL PROTECTED]> writes:
>
[...]
>> If we start talking about the C language, my opinion is that it's C
>> problem that it allows numeric comparison of "char" variables at
>> all. If one actually needs to compare alphabetic characters numerically,
>> he should first cast to required integer type.
>
> Char values are indexes in a character set. Taking the difference of
> indexes is legal. Other languages have ord('x'), but it's only an
> expensive typecast (byte('x') does exactly the same thing).

Why this typecast should be expensive? Is

   char c1, c2;
   ...
   if((unsigned char)c1 < (unsigned char)c2)

any more expensive than if(c1 < c2)?

[...]

>> Comparison of characters being numeric is not a very good property of
>> the C language.
>
> NACK, as above

The point was not to entirely disable it or to make it any more
expensive, but to somehow force programmer to be explicit that he indeed
needs numeric comparison of characters' indexes. Well, I do realize that
the above is not in the C "spirit" that is to allow implicit conversions
almost everywhere.

>> > I repeat: Thanks to using signed chars, the programs only
>> > work /by/ /accident/! Promoting the use of signed char strings is promoting
>> > bugs and endangering the stability of all our systems. You should stop this
>> > bullshit now, instead of increasing the pile.
>> 
>> Where did you see I promoted using of "singed char strings"?!
>
> char strings are often signed char strings, only using unsigned char 
> prevents this in a portable way. If you care about ordinal values of
> your strings, use unsigned.

It means that (as I don't want to change the type of my
strings/characters in case I suddenly start to care about ordinal values
of the characters) I must always use "unsigned char" for storing
characters in practice. Right?

There are at least three problems then:

1. The type of "abc" remains "char*", so effectively I'll have two
   different representations of strings.

2. I can't mix two different representations of strings in C without
   compromising type safety. In C, the type for characters is "char",
   that by definition is different type than "unsigned char".

3. As I now represent characters by unsigned tiny integers, I loose
   distinct type for unsigned tiny integers. Effectively this brings me
   back in time to the old days when C didn't have distinct type for
   signed tiny integers.

Therefore, your "solution" to one problem creates a bunch of
others. Worse yet, you in fact don't solve initial problem of comparing
strings. Take, for example, KOI8-R encoding. Comparing characters from
this encoding using "unsigned char" representation of character indexes
makes no more sense than comparing "signed char" representations, as
both comparisons will bring meaningless results, due to the fact that
the order of (some of) the characters in the table doesn't match their
alphabetical order.

>> If you don't like the fact that in C language characters are "char",
>> strings are "char*", and the sign of char is implementation-defined,
>> please argue with the C committee, not with me.
>> 
>> Or use -funsigned-char to get dialect of C that fits your requirements
>> better.
>
> If you restrict yourself to a specific C compiler, you are doomed, and 
> that's not a good start.

Isn't it you who insists that "char" should always be unsigned? It's
your problem then to use compiler that understands those imaginary
language that meets the requirement.

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Bodo Eggert [EMAIL PROTECTED] writes:
 On Fri, 16 Feb 2007, Sergei Organov wrote:
 Bodo Eggert [EMAIL PROTECTED] writes:
  Sergei Organov [EMAIL PROTECTED] wrote:
  Linus Torvalds [EMAIL PROTECTED] writes:

[...]
 If we start talking about the C language, my opinion is that it's C
 problem that it allows numeric comparison of char variables at
 all. If one actually needs to compare alphabetic characters numerically,
 he should first cast to required integer type.

 Char values are indexes in a character set. Taking the difference of
 indexes is legal. Other languages have ord('x'), but it's only an
 expensive typecast (byte('x') does exactly the same thing).

Why this typecast should be expensive? Is

   char c1, c2;
   ...
   if((unsigned char)c1  (unsigned char)c2)

any more expensive than if(c1  c2)?

[...]

 Comparison of characters being numeric is not a very good property of
 the C language.

 NACK, as above

The point was not to entirely disable it or to make it any more
expensive, but to somehow force programmer to be explicit that he indeed
needs numeric comparison of characters' indexes. Well, I do realize that
the above is not in the C spirit that is to allow implicit conversions
almost everywhere.

  I repeat: Thanks to using signed chars, the programs only
  work /by/ /accident/! Promoting the use of signed char strings is promoting
  bugs and endangering the stability of all our systems. You should stop this
  bullshit now, instead of increasing the pile.
 
 Where did you see I promoted using of singed char strings?!

 char strings are often signed char strings, only using unsigned char 
 prevents this in a portable way. If you care about ordinal values of
 your strings, use unsigned.

It means that (as I don't want to change the type of my
strings/characters in case I suddenly start to care about ordinal values
of the characters) I must always use unsigned char for storing
characters in practice. Right?

There are at least three problems then:

1. The type of abc remains char*, so effectively I'll have two
   different representations of strings.

2. I can't mix two different representations of strings in C without
   compromising type safety. In C, the type for characters is char,
   that by definition is different type than unsigned char.

3. As I now represent characters by unsigned tiny integers, I loose
   distinct type for unsigned tiny integers. Effectively this brings me
   back in time to the old days when C didn't have distinct type for
   signed tiny integers.

Therefore, your solution to one problem creates a bunch of
others. Worse yet, you in fact don't solve initial problem of comparing
strings. Take, for example, KOI8-R encoding. Comparing characters from
this encoding using unsigned char representation of character indexes
makes no more sense than comparing signed char representations, as
both comparisons will bring meaningless results, due to the fact that
the order of (some of) the characters in the table doesn't match their
alphabetical order.

 If you don't like the fact that in C language characters are char,
 strings are char*, and the sign of char is implementation-defined,
 please argue with the C committee, not with me.
 
 Or use -funsigned-char to get dialect of C that fits your requirements
 better.

 If you restrict yourself to a specific C compiler, you are doomed, and 
 that's not a good start.

Isn't it you who insists that char should always be unsigned? It's
your problem then to use compiler that understands those imaginary
language that meets the requirement.

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Rene Herman [EMAIL PROTECTED] writes:
[...]
 Given char's special nature, shouldn't the conclusion of this thread
 have long been simply that gcc needs -Wno-char-pointer-sign? (with
 whatever default, as far as I'm concerned).

I entirely agree that all the char business in C is messy enough to
justify separate warning switch(es) in GCC.

However, I still insist that the problem with the code:

   void foo(char *c);
   unsigned char *u;
   signed char *s;
   ...
   foo(u);
   foo(s);

is not (only) in signedness, as neither 'u' nor 's' has compatible type
with the char*, no matter what is the sign of char, so if one cares
about type safety he needs warnings on both invocations of foo().

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Bodo Eggert [EMAIL PROTECTED] writes:
 On Fri, 16 Feb 2007, Sergei Organov wrote:

[...]

 I'll say it again: Either the code using unspecified chars is correct, or 
 it isn't. If it's correct, neither using with signed nor with unsigned 
 chars is a bug and you should not warn at all, and if it's not correct, 
 you should always warn. Instead, gcc warns on code compiles for
 $arch.

Here is where we disagree. In my opinion, no matter what the sign of
char for given architecture is, it's incorrect to pass either signed
char* or unsigned char* argument to a function expecting char*.

  Therefore it's either always wrong to call your char* function with char*,
  unsigned char* _and_ signed char unless you can guarantee not to overflow 
  any
  of them, or it's always correct to call char* functions with any kind
  of these.
 
 How are you sure those who wrote foo(char*) agrees with your opinion or
 even understands all the involved issues?

 Let's asume we have this piece of buggy code. We compile it on an unsigned
 char architecture. No warning. *BOOM*

There should be warning, -- that's my point. char is different. char
is distinct from either signed char or unsigned char. Always. At
least it's how C is defined.

 Let's asume there is correct code, and we use it as designed:
 Warning: Wrong arch
 Warning: Wrong arch
 Warning: Wrong arch
 Warning: real issue 
 Warning: Wrong arch
 Warning: Wrong arch
 Warning: Wrong arch
 Warning: Wrong arch
 scroll off screen/
 Warning: Wrong arch
 Warning: Wrong arch
 Warning: Wrong arch
 Warning: Wrong arch
 Warning: Wrong arch

 You don't see real issue. *BOOM*


 What can you do about this warning? Let's asume we cast everywhere:

I already gave an answer in response to Linus:

static inline size_t ustrlen(const unsigned char *s)
{
return strlen((const char *)s);
}


 struct foo * p;
 printf(strlen(char*)p); *BOOM*

printf(ustrlen(p)); *NO BOOM*
unsigned char* u;
printf(ustrlen(u)); *NO WARNING*

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Linus Torvalds [EMAIL PROTECTED] writes:
 On Thu, 15 Feb 2007, Sergei Organov wrote:
 
 I agree that if the warning has no true positives, it sucks. The problem
 is that somehow I doubt it has none. And the reasons for the doubt are:

 Why do you harp on no true positives?

Because if somebody is capable to proof a warning has no true positives,
I immediately agree it's useless. I just wanted to check if it's indeed
the case. It seems it is not.

 That's a pointless thing. You can make *any* warning have true 
 positives. My personal favorite is the unconditional warning:

   warning: user is an idiot

 and I _guarantee_ you that it has a lot of true positives.

Yes, but there is obviously 0 correlation between the warning and the
user being an idiot (except that non-idiot will probably find a way to
either turn this warning off, or filter it out).

I already agreed that a warning may have true positives and still be
bad; and a warning having no false negatives is obviously a pretty
one, though unfortunately not that common in practice; and a warning
that has no true positives is useless. What we are arguing about are
intermediate cases that are most common in practice.

As programmers, we in fact are interested in correlation between a
warning and actual problem in the software, but for the kind of warnings
we are discussing (sign-correctness and type-safety in general), the
correlation depends on the style the program is written is, making it
difficult to use as a criteria in the discussion.

 It's the no false negatives angle you should look at.

I'd like to, but then I'll need to classify most of (all?) the GCC
warnings as bad, I'm afraid.

 THAT is what matters. The reason we don't see a lot of warnings about 
 idiot users is not that people don't do stupid things, but that 
 *sometimes* they actually don't do something stupid.

 Yeah, I know, it's far-fetched, but still.

 In other words, you're barking up *exactly* the wrong tree. You're looking 
 at it the wrong way around.

 Think of it this way: in science, a theory is proven to be bad by a single 
 undeniable fact just showing that it's wrong.

 The same is largely true of a warning. If the warning sometimes happens 
 for code that is perfectly fine, the warning is bad.

Consider:

int find_first_zero(const int *a)
{
int index = 0;
while(*a++ != 0)
index++;
return index;
}

unsigned int array[] = { 1, 2, 3, 0, 25, 14 };
int index = find_first_zero(array);  *WARNING*

So, by your logic, -Wpointer-sign warning is bad in general? If not,
then how will you fix the code above? And the upside of the fix is
what?

IMHO, the problem is that this warning is about violation of type
safety. Type safety by itself doesn't guarantee the code is perfectly
fine. Violation of type safety doesn't necessarily mean the code is
broken. A warning that warns about violation of type safety has the same
properties.  Now, it's up to you to decide if you want warnings on type
safety or not.

What you are saying, on the other hand, for me reads like this: I do
want warnings on violations of type safety in general, except for
signed char or unsigned char being used instead of char. I think
I do understand your reasons, -- I just don't agree with them.

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Bodo Eggert <[EMAIL PROTECTED]> writes:

> Sergei Organov <[EMAIL PROTECTED]> wrote:
>> Linus Torvalds <[EMAIL PROTECTED]> writes:
>
>>> Exactly because "char" *by*definition* is "indeterminate sign" as far as
>>> something like "strlen()" is concerned.
>> 
>> Thanks, I now understand that you either don't see the difference
>> between "indeterminate" and "implementation-defined" in this context or
>> consider it non-essential, so I think I've got your point.
>
> If you don't code for a specific compiler with specific settings, there is
> no implementation defining the signedness of char, and each part of the code
> using char* will be wrong unless it handles both cases correctly.

The problem here is that due to historical reasons, there could be code
out there that abuses "char" for "signed char" (not sure about "unsigned
char"). Old code and old habits are rather persistent.

> Therefore it's either always wrong to call your char* function with char*,
> unsigned char* _and_ signed char unless you can guarantee not to overflow any
> of them, or it's always correct to call char* functions with any kind
> of these.

How are you sure those who wrote foo(char*) agrees with your opinion or
even understands all the involved issues?

> Off cause if you happen to code for specific compiler settings, one signedness
> of char will become real and one warning will be legit. And if pigs fly, they
> should wear googles to protect their eyes ...

And if people were writing perfect portable code all the time, there
would be no need for warnings in the first place...

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Bodo Eggert <[EMAIL PROTECTED]> writes:
> Sergei Organov <[EMAIL PROTECTED]> wrote:
>> Linus Torvalds <[EMAIL PROTECTED]> writes:
[...]
> Using signed chars for strings is wrong in most countries on earth. It was
> wrong when the first IBM PC came out in 1981, and creating a compiler in
> 1987 defaulting to signed char is a sure sign of originating from an
> isolated country and knowing nothing about this planet.

Maybe I'm wrong, but wasn't it the case that C had no "signed char" type
that time, so "char" had to be abused for the "tiny signed integer"?

> Using signed chars in comparisons is especially wrong, and casting
> each char to unsigned before comparing them is likely to be
> forgotten.

If we start talking about the C language, my opinion is that it's C
problem that it allows numeric comparison of "char" variables at
all. If one actually needs to compare alphabetic characters numerically,
he should first cast to required integer type.

> Unsigned character strings are useless because there is no such thing
> as char(-23), and if these strings weren't casted to signed inside all
> IO functions, they wouldn't work correctly.

Didn't you swap "signed" and "unsigned" by mistake in this phrase? Or
are you indeed think that using "unsigned char*" for strings is useless?

> Only because many programmers don't compare chars, most programs will
> work outside the USA.

Comparison of characters being numeric is not a very good property of
the C language.

> I repeat: Thanks to using signed chars, the programs only
> work /by/ /accident/! Promoting the use of signed char strings is promoting
> bugs and endangering the stability of all our systems. You should stop this
> bullshit now, instead of increasing the pile.

Where did you see I promoted using of "singed char strings"?! If you
don't like the fact that in C language characters are "char", strings
are "char*", and the sign of char is implementation-defined, please
argue with the C committee, not with me.

Or use -funsigned-char to get dialect of C that fits your requirements
better.

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Bodo Eggert [EMAIL PROTECTED] writes:
 Sergei Organov [EMAIL PROTECTED] wrote:
 Linus Torvalds [EMAIL PROTECTED] writes:
[...]
 Using signed chars for strings is wrong in most countries on earth. It was
 wrong when the first IBM PC came out in 1981, and creating a compiler in
 1987 defaulting to signed char is a sure sign of originating from an
 isolated country and knowing nothing about this planet.

Maybe I'm wrong, but wasn't it the case that C had no signed char type
that time, so char had to be abused for the tiny signed integer?

 Using signed chars in comparisons is especially wrong, and casting
 each char to unsigned before comparing them is likely to be
 forgotten.

If we start talking about the C language, my opinion is that it's C
problem that it allows numeric comparison of char variables at
all. If one actually needs to compare alphabetic characters numerically,
he should first cast to required integer type.

 Unsigned character strings are useless because there is no such thing
 as char(-23), and if these strings weren't casted to signed inside all
 IO functions, they wouldn't work correctly.

Didn't you swap signed and unsigned by mistake in this phrase? Or
are you indeed think that using unsigned char* for strings is useless?

 Only because many programmers don't compare chars, most programs will
 work outside the USA.

Comparison of characters being numeric is not a very good property of
the C language.

 I repeat: Thanks to using signed chars, the programs only
 work /by/ /accident/! Promoting the use of signed char strings is promoting
 bugs and endangering the stability of all our systems. You should stop this
 bullshit now, instead of increasing the pile.

Where did you see I promoted using of singed char strings?! If you
don't like the fact that in C language characters are char, strings
are char*, and the sign of char is implementation-defined, please
argue with the C committee, not with me.

Or use -funsigned-char to get dialect of C that fits your requirements
better.

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Bodo Eggert [EMAIL PROTECTED] writes:

 Sergei Organov [EMAIL PROTECTED] wrote:
 Linus Torvalds [EMAIL PROTECTED] writes:

 Exactly because char *by*definition* is indeterminate sign as far as
 something like strlen() is concerned.
 
 Thanks, I now understand that you either don't see the difference
 between indeterminate and implementation-defined in this context or
 consider it non-essential, so I think I've got your point.

 If you don't code for a specific compiler with specific settings, there is
 no implementation defining the signedness of char, and each part of the code
 using char* will be wrong unless it handles both cases correctly.

The problem here is that due to historical reasons, there could be code
out there that abuses char for signed char (not sure about unsigned
char). Old code and old habits are rather persistent.

 Therefore it's either always wrong to call your char* function with char*,
 unsigned char* _and_ signed char unless you can guarantee not to overflow any
 of them, or it's always correct to call char* functions with any kind
 of these.

How are you sure those who wrote foo(char*) agrees with your opinion or
even understands all the involved issues?

 Off cause if you happen to code for specific compiler settings, one signedness
 of char will become real and one warning will be legit. And if pigs fly, they
 should wear googles to protect their eyes ...

And if people were writing perfect portable code all the time, there
would be no need for warnings in the first place...

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Olivier Galibert <[EMAIL PROTECTED]> writes:

> On Tue, Feb 13, 2007 at 09:06:24PM +0300, Sergei Organov wrote:
>> I agree that making strxxx() family special is not a good idea. So what
>> do we do for a random foo(char*) called with an 'unsigned char*'
>> argument? Silence? Hmmm... It's not immediately obvious that it's indeed
>> harmless. Yet another -Wxxx option to GCC to silence this particular
>> case?
>
> Silence would be good.  "char *" has a special status in C, it can be:
> - pointer to a char/to an array of chars (standard interpretation)
> - pointer to a string
> - generic pointer to memory you can read(/write)
>
> Check the aliasing rules if you don't believe be on the third one.

Generic pointer still reads 'void*' isn't it? If you don't believe me,
check memcpu() and friends. From the POV of aliasing rules, "char*",
"unsigned char*", and "signed char*" are all the same, as aliasing rules
mention "character type", that according to the standard, is defined
like this:

  "The three types char, signed char, and unsigned char are collectively
   called the character types."

> And it's *way* more often cases 2 and 3 than 1 for the simple reason
> that the signedness of char is unpredictable.

Are the first two different other than by 0-termination? If not, then
differentiate 1 and 2 by signedness doesn't make sense. If we throw
away signedness argument, then yes, 2 is more common than 1, due to
obvious reason that it's much more convenient to use 0-terminated arrays
of characters in C than plain arrays of characters.

For 3, I'd use "unsigned char*" (or, sometimes, maybe, "signed char*") to
do actual access (due to aliasing rules), but "void*" as an argument
type, as arbitrary pointer is spelled "void*" in C.

So, it's 2 that I'd consider the most common usage for "char*". Strings
of characters are idiomatically "char*" in C.

> As a result, a signedness warning between char * and (un)signed char *
> is 99.99% of the time stupid.

As a result of what? Of the fact that strings in C are "char*"? Of the
fact that one can use "char*" to access arbitrary memory? If the latter,
then "char*", "signed char*", and "unsigned char*" are all equal, and
you may well argue that there should be no warning between "signed
char*" and "unsigned char*" as well, as either of them could be used to
access memory.

Anyway, I've already stated that I don't think it's signedness that matters
here. Standard explicitly says "char", "signed char", and "unsigned
char" are all distinct types, so the warning should be about
incompatible pointer instead. Anyway, I even don't dispute those 99.99%
number, I just want somebody to explain me how dangerous those 0.001%
are, and isn't it exactly 0%?

[...]
>> I'm afraid I don't follow. Do we have a way to say "I want an int of
>> indeterminate sign" in C?
>
> Almost completely.  The rules on aliasing say you can convert pointer
> between signed and unsigned variants and the accesses will be
> unsurprising.

Only from the point of view of aliasing. Aliasing rules don't disallow
other surprises.

>
> The only problem is that the implicit conversion of incompatible
> pointer parameters to a function looks impossible in the draft I have.

Why do you think it's impossible according to the draft?

> Probably has been corrected in the final version.

I doubt it's impossible either in the draft or in the final version.

> In any case, having for instance unsigned int * in a prototype really
> means in the language "I want a pointer to integers, and I'm probably
> going to use it them as unsigned, so beware".

> For the special case of char, since the beware version would require a
> signed or unsigned tag, it really means indeterminate.

IMHO, it would read "I want a pointer to alphabetic characters, and I'm
probably going to use them as alphabetic characters, so beware". No
signedness involved.

For example, suppose we have 3 function that compare two
zero-terminated arrays:

int compare1(char *c1, char *c2);
int compare2(unsigned char *c1, unsigned char *c2);
int compare3(signed char *c1, signed char *c2);

the first one might compare arguments alphabetically, while the second
and the third compare them numerically. All of the 3 are different, so
passing wrong type of argument to either of them might be dangerous.

> C is sometimes called a high-level assembler for a reason :-)

I'm afraid that those times when it actually was, are far in the past,
for better or worse.

>> The same way there doesn't seem to be a way to say "I want a char of
>> indeterminate sign". :( So no, strlen() doesn't

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Linus Torvalds <[EMAIL PROTECTED]> writes:
> On Thu, 15 Feb 2007, Sergei Organov wrote:

[...Skip things I agree with...]

>> > But if you have 
>> >
>> >unsigned char *mystring;
>> >
>> >len = strlen(mystring);
>> >
>> > then please tell me how to fix that warning without making the code 
>> > *worse* from a type-safety standpoint? You CANNOT. You'd have to cast it 
>> > explicitly (or assing it through a "void *" variable), both of which 
>> > actually MAKE THE TYPE-CHECKING PROBLEM WORSE!
>> 
>> Because instead of trying to fix the code, you insist on hiding the
>> warning.
>
> No. I'm saying that I have *reasons* that I need my string to be unsigned. 
> That makes your first "fix" totally bogus:

Somehow I still suppose that most of those warnings could be fixed by
using "char*" instead. Yes, I believe you, you have reasons. But still I
only heard about isxxx() that appeared to be not a reason in practice
(in the context of the kernel tree). As you didn't tell about other
reasons, my expectation is that they are not in fact very common. I'm
curious what are actual reasons besides the fact that quite a lot of
code is apparently written this way in the kernel. The latter is indeed
a very sound reason for the warning to be sucks for kernel developers,
but it might be not a reason for it to be sucks in general.

>> There are at least two actual fixes:
>> 
>> 1. Do 'char *mystring' instead, as otherwise compiler can't figure out
>>that you are going to use 'mystring' to point to zero-terminated
>>array of characters.
>
> And the second fix is a fix, but it is, again, worse than the warning:
>
>> 2. Use "len = ustrlen(mystring)" if you indeed need something like C
>>strings
>
> Sure, I can do that, but the upside is what?

1. You make it explicit, in a type-safe manner, that you are sure it's
   safe to call "strlen()" with "unsigned char*" argument. We all agree
   about it. Let's write it down in the program. And we can already add
   "strcmp()" to the list. And, say, ustrdup() and ustrchr() returning
   "unsigned char *", would be more type-safe either.

2. You make it more explicit that you indeed mean to use your own
   representation of strings that is different than what C idiomatically
   uses for strings, along with explicitly defined allowed set of
   operations on this representation.

The actual divergence of opinion seems to be that you are sure it's safe
to call any foo(char*) with "unsigned char*" argument, and I'm not, due
to the reasons described below. If you are right, then there is indeed
little or no point in doing this work.

> Getting rid of a warning that was bogus to begin with?

I agree that if the warning has no true positives, it sucks. The problem
is that somehow I doubt it has none. And the reasons for the doubt are:

1. C standard does explicitly say that "char" is different type from
   either "signed char" or "unsigned char". There should be some reason
   for that, otherwise they'd simply write that "char" is a synonym for
   one of "signed char" or "unsigned char", depending on implementation.

2. If "char" in fact matches one of the other two types for given
   architecture, it is definitely different from another one, so
   substituting "char" for the different one might be unsafe. As a
   consequence, for a portable program it might be unsafe to substitute
   "char" for any of signed or unsigned, as "char" may happen to
   differ from any of them.

While the doubts aren't resolved, I prefer to at least be careful with
things that I don't entirely understand, so I wish compiler give me a
warning about subtle semantic differences (if any).

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Linus Torvalds <[EMAIL PROTECTED]> writes:
> On Tue, 13 Feb 2007, Sergei Organov wrote:
[...]
> BUT (and this is a big but) within the discussion of "strlen()", that is 
> no longer true. "strlen()" exists _outside_ of a single particular 
> implementation. As such, "implementation-defined" is no longer something 
> that "strlen()" can depend on.
>
> As an example of this argument, try this:
>
>   #include 
>   #include 
>
>   int main(int argc, char **argv)
>   {
>   char a1[] = { -1, 0 }, a2[] = { 1, 0 };
>
>   printf("%d %d\n", a1[0] < a2[0], strcmp(a1, a2) < 0);
>   return 0;
>   }
>
> and *before* you compile it, try to guess what the output is.

Well, I'll try to play fair, so I didn't yet compile it. Now, strcmp()
is defined in the C standard so that its behavior doesn't depend on the
sign of char:

   "The sign of a nonzero value returned by the comparison functions
memcmp, strcmp, and strncmp is determined by the sign of the
difference between the values of the first pair of characters (both
interpreted as unsigned char) that differ in the objects being

compared."

[Therefore, at least I don't need to build GCC multilib'ed on
-fsigned/unsigned-char to get consistent results even if strcmp() in
fact lives in a library, that was my first thought before I referred to
the standard ;)]

Suppose the char is signed. Then a1[0] < a2[0] (= -1 < 1) should be
true. On 2's-complement implementation with 8bit char, -1 converted by
strcmp() to unsigned char should be 0xFF, and 1 converted should be
1. So strcmp() should be equivalent to (0xFF < 1) that is false. So
I'd expect

 1 0

result on implementation with signed char.

Now suppose the char is unsigned. Then on 2's-complement implementation
with 8bit-byte CPU, a1[0] should be 0xFF, and a2[0] should be 1. The
result from strcmp() won't change. So I'd expect

 0 0

result on implementation with unsigned char.

Now I'm going to compile it (I must admit I'm slightly afraid to get
surprising results, so I've re-read my above reasonings before
compiling):

[EMAIL PROTECTED] tmp$ cat strcmp.c
#include 
#include 
int main()
{
  char a1[] = { -1, 0 }, a2[] = { 1, 0 };

  printf("%d %d\n", a1[0] < a2[0], strcmp(a1, a2) < 0);
  return 0;
}
[EMAIL PROTECTED] tmp$ gcc -v
Using built-in specs.
Target: i486-linux-gnu
...
gcc version 4.1.2 20061028 (prerelease) (Debian 4.1.1-19)
[EMAIL PROTECTED] tmp$ gcc -O2 strcmp.c -o strcmp && ./strcmp
1 0

> And when that confuses you,

It didn't, or did I miss something? Is char unsigned by default?

> try to compile it using gcc with the 
> "-funsigned-char" flag (or "-fsigned-char" if you started out on an 
> architecture where char was unsigned by default)

[EMAIL PROTECTED] tmp$ gcc -O2 -fsigned-char strcmp.c -o strcmp && ./strcmp
1 0
[EMAIL PROTECTED] tmp$ gcc -O2 -funsigned-char strcmp.c -o strcmp && ./strcmp
0 0
[EMAIL PROTECTED] tmp$

Due to above, apparently char is indeed signed by default, so what?

> And when you really *really* think about it afterwards, I think you'll go 
> "Ahh.. Linus is right". It's more than "implementation-defined": it really 
> is totally indeterminate for code like this.

The fact is that strcmp() is explicitly defined in the C standard so
that it will bring the same result no matter what the sign of "char"
type is. Therefore, it obviously can't be used to determine the sign of
"char", so from the POV of usage of strcmp(), the sign of its argument
is indeed "indeterminate". What I fail to see is how this fact could
help in proving that for any function taking "char*" argument the sign
of char is indeterminate.

Anyway, it seems that you still miss (or ignore) my point that it's not
(only) sign of "char" that makes it suspect to call functions requesting
"char*" argument with "unsigned char*" value.

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Linus Torvalds <[EMAIL PROTECTED]> writes:
> On Tue, 13 Feb 2007, Sergei Organov wrote:
>> 
>> Sorry, what do you do with "variable 'xxx' might be used uninitialized"
>> warning when it's false? Turn it off? Annotate the source? Assign fake
>> initialization value? Change the compiler so that it does "the effort"
>> for you? Never encountered false positive from this warning?
>
> The thing is, that warning is at least easy to shut up. 
>
> You just add a fake initialization. There isn't any real downside.

Yes, there is. There are at least 2 drawbacks. Minor one is
initialization eating cycles. Major one is that if later I change the
code and there will appear a pass that by mistake uses those fake value,
I won't get the warning anymore. The latter drawback is somewhat similar
to the drawbacks of explicit type casts.

[I'd, personally, try to do code reorganization instead so that it
becomes obvious for the compiler that the variable can't be used
uninitialized. Quite often it makes the code better, at least it was my
experience so far.]

> In contrast, to shut up the idiotic pointer-sign warning, you have to add 
> a cast.

[Did I already say that I think it's wrong warning to be given in this
particular case, as the problem with the code is not in signedness?]

1. Don't try to hide the warning, at least not immediately, -- consider
   fixing the code first. Ah, sorry, you've already decided for yourself
   that the warning is idiotic, so there is no reason to try to...

2. If you add a cast, it's not in contrast, it's rather similar to fake
   initialization above as they have somewhat similar drawback.

> Now, some people seem to think that adding casts is a way of life, and 
> think that C programs always have a lot of casts.

Hopefully I'm not one of those.

[...]
> But if you have 
>
>   unsigned char *mystring;
>
>   len = strlen(mystring);
>
> then please tell me how to fix that warning without making the code 
> *worse* from a type-safety standpoint? You CANNOT. You'd have to cast it 
> explicitly (or assing it through a "void *" variable), both of which 
> actually MAKE THE TYPE-CHECKING PROBLEM WORSE!

Because instead of trying to fix the code, you insist on hiding the
warning. There are at least two actual fixes:

1. Do 'char *mystring' instead, as otherwise compiler can't figure out
   that you are going to use 'mystring' to point to zero-terminated
   array of characters.

2. Use "len = ustrlen(mystring)" if you indeed need something like C
   strings, but built from "unsigned char" elements. Similar to (1), this
   will explain your intent to those stupid compiler. Should I also
   mention that due to the definition of strlen(), the implementation of
   ustrlen() is a one-liner (thnx Andrew):

static inline size_t ustrlen(const unsigned char *s)
{
return strlen((const char *)s);
}

Overall, describe your intent to the compiler more clearly, as reading
programmer's mind is not the kind of business where compilers are
strong.

> See? The warning made no sense to begin with, and it warns about something 
> where the alternatives are worse than what it warned about.
>
> Ergo. It's a crap warning.

No, I'm still not convinced. Well, I'll try to explain my point
differently. What would you think about the following line, should you
encounter it in real code:

  len = strlen(my_tiny_integers);

I'd think that as 'my_tiny_integers' is used as an argument to strlen(),
it might be zero-terminated array of characters. But why does it have
such a name then?!  Maybe it in fact holds an array of integers where 0
is not necessarily terminating, and the intent of this code is just to
find first occurrence of 0? It's confusing and I'd probably go check the
declaration. Declaration happens to be "unsigned char *my_tiny_integers"
that at least is rather consistent with the variable name, but doesn't
resolve my initial doubts. So I need to go check other usages of this
'my_tiny_integers' variable to figure out what's actually going on
here. Does it sound as a good programming practice? I don't think so.

Now, you probably realize that in the example you gave above you've
effectively declared "mystring" to be a pointer to
"tiny_unsigned_integer". As compiler (fortunately) doesn't guess intent
from variable names, it can't give us warning at the point of
declaration, but I still think we must be thankful that it gave us a
warning (even though gcc gives a wrong kind of warning) at the point of
dubious use.

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Linus Torvalds [EMAIL PROTECTED] writes:
 On Tue, 13 Feb 2007, Sergei Organov wrote:
 
 Sorry, what do you do with variable 'xxx' might be used uninitialized
 warning when it's false? Turn it off? Annotate the source? Assign fake
 initialization value? Change the compiler so that it does the effort
 for you? Never encountered false positive from this warning?

 The thing is, that warning is at least easy to shut up. 

 You just add a fake initialization. There isn't any real downside.

Yes, there is. There are at least 2 drawbacks. Minor one is
initialization eating cycles. Major one is that if later I change the
code and there will appear a pass that by mistake uses those fake value,
I won't get the warning anymore. The latter drawback is somewhat similar
to the drawbacks of explicit type casts.

[I'd, personally, try to do code reorganization instead so that it
becomes obvious for the compiler that the variable can't be used
uninitialized. Quite often it makes the code better, at least it was my
experience so far.]

 In contrast, to shut up the idiotic pointer-sign warning, you have to add 
 a cast.

[Did I already say that I think it's wrong warning to be given in this
particular case, as the problem with the code is not in signedness?]

1. Don't try to hide the warning, at least not immediately, -- consider
   fixing the code first. Ah, sorry, you've already decided for yourself
   that the warning is idiotic, so there is no reason to try to...

2. If you add a cast, it's not in contrast, it's rather similar to fake
   initialization above as they have somewhat similar drawback.

 Now, some people seem to think that adding casts is a way of life, and 
 think that C programs always have a lot of casts.

Hopefully I'm not one of those.

[...]
 But if you have 

   unsigned char *mystring;

   len = strlen(mystring);

 then please tell me how to fix that warning without making the code 
 *worse* from a type-safety standpoint? You CANNOT. You'd have to cast it 
 explicitly (or assing it through a void * variable), both of which 
 actually MAKE THE TYPE-CHECKING PROBLEM WORSE!

Because instead of trying to fix the code, you insist on hiding the
warning. There are at least two actual fixes:

1. Do 'char *mystring' instead, as otherwise compiler can't figure out
   that you are going to use 'mystring' to point to zero-terminated
   array of characters.

2. Use len = ustrlen(mystring) if you indeed need something like C
   strings, but built from unsigned char elements. Similar to (1), this
   will explain your intent to those stupid compiler. Should I also
   mention that due to the definition of strlen(), the implementation of
   ustrlen() is a one-liner (thnx Andrew):

static inline size_t ustrlen(const unsigned char *s)
{
return strlen((const char *)s);
}

Overall, describe your intent to the compiler more clearly, as reading
programmer's mind is not the kind of business where compilers are
strong.

 See? The warning made no sense to begin with, and it warns about something 
 where the alternatives are worse than what it warned about.

 Ergo. It's a crap warning.

No, I'm still not convinced. Well, I'll try to explain my point
differently. What would you think about the following line, should you
encounter it in real code:

  len = strlen(my_tiny_integers);

I'd think that as 'my_tiny_integers' is used as an argument to strlen(),
it might be zero-terminated array of characters. But why does it have
such a name then?!  Maybe it in fact holds an array of integers where 0
is not necessarily terminating, and the intent of this code is just to
find first occurrence of 0? It's confusing and I'd probably go check the
declaration. Declaration happens to be unsigned char *my_tiny_integers
that at least is rather consistent with the variable name, but doesn't
resolve my initial doubts. So I need to go check other usages of this
'my_tiny_integers' variable to figure out what's actually going on
here. Does it sound as a good programming practice? I don't think so.

Now, you probably realize that in the example you gave above you've
effectively declared mystring to be a pointer to
tiny_unsigned_integer. As compiler (fortunately) doesn't guess intent
from variable names, it can't give us warning at the point of
declaration, but I still think we must be thankful that it gave us a
warning (even though gcc gives a wrong kind of warning) at the point of
dubious use.

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Linus Torvalds [EMAIL PROTECTED] writes:
 On Tue, 13 Feb 2007, Sergei Organov wrote:
[...]
 BUT (and this is a big but) within the discussion of strlen(), that is 
 no longer true. strlen() exists _outside_ of a single particular 
 implementation. As such, implementation-defined is no longer something 
 that strlen() can depend on.

 As an example of this argument, try this:

   #include string.h
   #include stdio.h

   int main(int argc, char **argv)
   {
   char a1[] = { -1, 0 }, a2[] = { 1, 0 };

   printf(%d %d\n, a1[0]  a2[0], strcmp(a1, a2)  0);
   return 0;
   }

 and *before* you compile it, try to guess what the output is.

Well, I'll try to play fair, so I didn't yet compile it. Now, strcmp()
is defined in the C standard so that its behavior doesn't depend on the
sign of char:

   The sign of a nonzero value returned by the comparison functions
memcmp, strcmp, and strncmp is determined by the sign of the
difference between the values of the first pair of characters (both
interpreted as unsigned char) that differ in the objects being

compared.

[Therefore, at least I don't need to build GCC multilib'ed on
-fsigned/unsigned-char to get consistent results even if strcmp() in
fact lives in a library, that was my first thought before I referred to
the standard ;)]

Suppose the char is signed. Then a1[0]  a2[0] (= -1  1) should be
true. On 2's-complement implementation with 8bit char, -1 converted by
strcmp() to unsigned char should be 0xFF, and 1 converted should be
1. So strcmp() should be equivalent to (0xFF  1) that is false. So
I'd expect

 1 0

result on implementation with signed char.

Now suppose the char is unsigned. Then on 2's-complement implementation
with 8bit-byte CPU, a1[0] should be 0xFF, and a2[0] should be 1. The
result from strcmp() won't change. So I'd expect

 0 0

result on implementation with unsigned char.

Now I'm going to compile it (I must admit I'm slightly afraid to get
surprising results, so I've re-read my above reasonings before
compiling):

[EMAIL PROTECTED] tmp$ cat strcmp.c
#include stdio.h
#include string.h
int main()
{
  char a1[] = { -1, 0 }, a2[] = { 1, 0 };

  printf(%d %d\n, a1[0]  a2[0], strcmp(a1, a2)  0);
  return 0;
}
[EMAIL PROTECTED] tmp$ gcc -v
Using built-in specs.
Target: i486-linux-gnu
...
gcc version 4.1.2 20061028 (prerelease) (Debian 4.1.1-19)
[EMAIL PROTECTED] tmp$ gcc -O2 strcmp.c -o strcmp  ./strcmp
1 0

 And when that confuses you,

It didn't, or did I miss something? Is char unsigned by default?

 try to compile it using gcc with the 
 -funsigned-char flag (or -fsigned-char if you started out on an 
 architecture where char was unsigned by default)

[EMAIL PROTECTED] tmp$ gcc -O2 -fsigned-char strcmp.c -o strcmp  ./strcmp
1 0
[EMAIL PROTECTED] tmp$ gcc -O2 -funsigned-char strcmp.c -o strcmp  ./strcmp
0 0
[EMAIL PROTECTED] tmp$

Due to above, apparently char is indeed signed by default, so what?

 And when you really *really* think about it afterwards, I think you'll go 
 Ahh.. Linus is right. It's more than implementation-defined: it really 
 is totally indeterminate for code like this.

The fact is that strcmp() is explicitly defined in the C standard so
that it will bring the same result no matter what the sign of char
type is. Therefore, it obviously can't be used to determine the sign of
char, so from the POV of usage of strcmp(), the sign of its argument
is indeed indeterminate. What I fail to see is how this fact could
help in proving that for any function taking char* argument the sign
of char is indeterminate.

Anyway, it seems that you still miss (or ignore) my point that it's not
(only) sign of char that makes it suspect to call functions requesting
char* argument with unsigned char* value.

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Linus Torvalds [EMAIL PROTECTED] writes:
 On Thu, 15 Feb 2007, Sergei Organov wrote:

[...Skip things I agree with...]

  But if you have 
 
 unsigned char *mystring;
 
 len = strlen(mystring);
 
  then please tell me how to fix that warning without making the code 
  *worse* from a type-safety standpoint? You CANNOT. You'd have to cast it 
  explicitly (or assing it through a void * variable), both of which 
  actually MAKE THE TYPE-CHECKING PROBLEM WORSE!
 
 Because instead of trying to fix the code, you insist on hiding the
 warning.

 No. I'm saying that I have *reasons* that I need my string to be unsigned. 
 That makes your first fix totally bogus:

Somehow I still suppose that most of those warnings could be fixed by
using char* instead. Yes, I believe you, you have reasons. But still I
only heard about isxxx() that appeared to be not a reason in practice
(in the context of the kernel tree). As you didn't tell about other
reasons, my expectation is that they are not in fact very common. I'm
curious what are actual reasons besides the fact that quite a lot of
code is apparently written this way in the kernel. The latter is indeed
a very sound reason for the warning to be sucks for kernel developers,
but it might be not a reason for it to be sucks in general.

 There are at least two actual fixes:
 
 1. Do 'char *mystring' instead, as otherwise compiler can't figure out
that you are going to use 'mystring' to point to zero-terminated
array of characters.

 And the second fix is a fix, but it is, again, worse than the warning:

 2. Use len = ustrlen(mystring) if you indeed need something like C
strings

 Sure, I can do that, but the upside is what?

1. You make it explicit, in a type-safe manner, that you are sure it's
   safe to call strlen() with unsigned char* argument. We all agree
   about it. Let's write it down in the program. And we can already add
   strcmp() to the list. And, say, ustrdup() and ustrchr() returning
   unsigned char *, would be more type-safe either.

2. You make it more explicit that you indeed mean to use your own
   representation of strings that is different than what C idiomatically
   uses for strings, along with explicitly defined allowed set of
   operations on this representation.

The actual divergence of opinion seems to be that you are sure it's safe
to call any foo(char*) with unsigned char* argument, and I'm not, due
to the reasons described below. If you are right, then there is indeed
little or no point in doing this work.

 Getting rid of a warning that was bogus to begin with?

I agree that if the warning has no true positives, it sucks. The problem
is that somehow I doubt it has none. And the reasons for the doubt are:

1. C standard does explicitly say that char is different type from
   either signed char or unsigned char. There should be some reason
   for that, otherwise they'd simply write that char is a synonym for
   one of signed char or unsigned char, depending on implementation.

2. If char in fact matches one of the other two types for given
   architecture, it is definitely different from another one, so
   substituting char for the different one might be unsafe. As a
   consequence, for a portable program it might be unsafe to substitute
   char for any of signed or unsigned, as char may happen to
   differ from any of them.

While the doubts aren't resolved, I prefer to at least be careful with
things that I don't entirely understand, so I wish compiler give me a
warning about subtle semantic differences (if any).

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Olivier Galibert [EMAIL PROTECTED] writes:

 On Tue, Feb 13, 2007 at 09:06:24PM +0300, Sergei Organov wrote:
 I agree that making strxxx() family special is not a good idea. So what
 do we do for a random foo(char*) called with an 'unsigned char*'
 argument? Silence? Hmmm... It's not immediately obvious that it's indeed
 harmless. Yet another -Wxxx option to GCC to silence this particular
 case?

 Silence would be good.  char * has a special status in C, it can be:
 - pointer to a char/to an array of chars (standard interpretation)
 - pointer to a string
 - generic pointer to memory you can read(/write)

 Check the aliasing rules if you don't believe be on the third one.

Generic pointer still reads 'void*' isn't it? If you don't believe me,
check memcpu() and friends. From the POV of aliasing rules, char*,
unsigned char*, and signed char* are all the same, as aliasing rules
mention character type, that according to the standard, is defined
like this:

  The three types char, signed char, and unsigned char are collectively
   called the character types.

 And it's *way* more often cases 2 and 3 than 1 for the simple reason
 that the signedness of char is unpredictable.

Are the first two different other than by 0-termination? If not, then
differentiate 1 and 2 by signedness doesn't make sense. If we throw
away signedness argument, then yes, 2 is more common than 1, due to
obvious reason that it's much more convenient to use 0-terminated arrays
of characters in C than plain arrays of characters.

For 3, I'd use unsigned char* (or, sometimes, maybe, signed char*) to
do actual access (due to aliasing rules), but void* as an argument
type, as arbitrary pointer is spelled void* in C.

So, it's 2 that I'd consider the most common usage for char*. Strings
of characters are idiomatically char* in C.

 As a result, a signedness warning between char * and (un)signed char *
 is 99.99% of the time stupid.

As a result of what? Of the fact that strings in C are char*? Of the
fact that one can use char* to access arbitrary memory? If the latter,
then char*, signed char*, and unsigned char* are all equal, and
you may well argue that there should be no warning between signed
char* and unsigned char* as well, as either of them could be used to
access memory.

Anyway, I've already stated that I don't think it's signedness that matters
here. Standard explicitly says char, signed char, and unsigned
char are all distinct types, so the warning should be about
incompatible pointer instead. Anyway, I even don't dispute those 99.99%
number, I just want somebody to explain me how dangerous those 0.001%
are, and isn't it exactly 0%?

[...]
 I'm afraid I don't follow. Do we have a way to say I want an int of
 indeterminate sign in C?

 Almost completely.  The rules on aliasing say you can convert pointer
 between signed and unsigned variants and the accesses will be
 unsurprising.

Only from the point of view of aliasing. Aliasing rules don't disallow
other surprises.


 The only problem is that the implicit conversion of incompatible
 pointer parameters to a function looks impossible in the draft I have.

Why do you think it's impossible according to the draft?

 Probably has been corrected in the final version.

I doubt it's impossible either in the draft or in the final version.

 In any case, having for instance unsigned int * in a prototype really
 means in the language I want a pointer to integers, and I'm probably
 going to use it them as unsigned, so beware.

 For the special case of char, since the beware version would require a
 signed or unsigned tag, it really means indeterminate.

IMHO, it would read I want a pointer to alphabetic characters, and I'm
probably going to use them as alphabetic characters, so beware. No
signedness involved.

For example, suppose we have 3 function that compare two
zero-terminated arrays:

int compare1(char *c1, char *c2);
int compare2(unsigned char *c1, unsigned char *c2);
int compare3(signed char *c1, signed char *c2);

the first one might compare arguments alphabetically, while the second
and the third compare them numerically. All of the 3 are different, so
passing wrong type of argument to either of them might be dangerous.

 C is sometimes called a high-level assembler for a reason :-)

I'm afraid that those times when it actually was, are far in the past,
for better or worse.

 The same way there doesn't seem to be a way to say I want a char of
 indeterminate sign. :( So no, strlen() doesn't actually say that, no
 matter if we like it or not. It actually says I want a char with
 implementation-defined sign.

 In this day and age it means I want a 0-terminated string.

IMHO, you've forgot to say of alphabetic characters.

 Everything else is explicitely signed char * or unsigned char *, often
 through typedefs in the signed case.

Those are for signed and unsigned tiny integers, with confusing names,
that in turn are just historical baggage.

 In fact it's implementation-defined

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Olivier Galibert <[EMAIL PROTECTED]> writes:
> On Tue, Feb 13, 2007 at 09:06:24PM +0300, Sergei Organov wrote:
[...]
>> May I suggest another definition for a warning being entirely sucks?
>> "The warning is entirely sucks if and only if it never has true
>> positives." In all other cases it's only more or less sucks, IMHO.
>
> That means a warning that triggers on every line saying "there may be
> a bug there" does not entirely suck?

You got me. This warning is indeed entirely sucks. My definition is
poor.

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Olivier Galibert [EMAIL PROTECTED] writes:
 On Tue, Feb 13, 2007 at 09:06:24PM +0300, Sergei Organov wrote:
[...]
 May I suggest another definition for a warning being entirely sucks?
 The warning is entirely sucks if and only if it never has true
 positives. In all other cases it's only more or less sucks, IMHO.

 That means a warning that triggers on every line saying there may be
 a bug there does not entirely suck?

You got me. This warning is indeed entirely sucks. My definition is
poor.

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

"Pekka Enberg" <[EMAIL PROTECTED]> writes:
> On 2/13/07, Sergei Organov <[EMAIL PROTECTED]> wrote:
>> With almost any warning out there one makes more or less efforts to
>> suppress the warning where it gives false positives, isn't it?
>
> Yes, as long it's the _compiler_ that's doing the effort.
> You shouldn't need to annotate the source just to make the compiler
> shut up.

Sorry, what do you do with "variable 'xxx' might be used uninitialized"
warning when it's false? Turn it off? Annotate the source? Assign fake
initialization value? Change the compiler so that it does "the effort"
for you? Never encountered false positive from this warning?

> Once the compiler starts issuing enough false positives, it's
> time to turn off that warning completely.

Yes, I don't argue that. I said "otherwise the warning is more or less
sucks", and then it's up to programmers to decide if it's enough sucks
to be turned off. The decision depends on the importance of its true
positives then. Only if warning never has true positives it is
unconditional, total, unhelpful crap, -- that was my point.

> Therefore, the only sane strategy for a warning is to aim for zero
> false positives.

Sure. But unfortunately this in an unreachable aim in most cases.

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Linus Torvalds <[EMAIL PROTECTED]> writes:
> On Tue, 13 Feb 2007, Sergei Organov wrote:
>> >
>> >"I want a char of indeterminate sign"!
>> 
>> I'm afraid I don't follow. Do we have a way to say "I want an int of
>> indeterminate sign" in C? The same way there doesn't seem to be a way
>> to say "I want a char of indeterminate sign".
>
> You're wrong.

Sure, I knew it from the very beginning ;)

>
> Exactly because "char" *by*definition* is "indeterminate sign" as far as 
> something like "strlen()" is concerned.

Thanks, I now understand that you either don't see the difference
between "indeterminate" and "implementation-defined" in this context or
consider it non-essential, so I think I've got your point.

> "char" is _special_. Char is _different_. Char is *not* "int".

Sure.

>
>> So no, strlen() doesn't actually say that, no matter if we like it or 
>> not. It actually says "I want a char with implementation-defined sign".
>
> You're arguing some strange semantic difference in the English
> language.

Didn't I further explain what I meant exactly (that you've skipped)?
OK, never mind.

> I'm not really interested.

OK, no problem.

>
> THE FACT IS, THAT "strlen()" IS DEFINED UNIVERSALLY AS TAKING "char *".

So just don't pass it "unsigned char*". End of story.

>
> That BY DEFINITION means that "strlen()" cannot care about the sign, 
> because the sign IS NOT DEFINED UNIVERSALLY!
>
> And if you cannot accept that fact, it's your problem. Not mine.

I never had problems either with strlen() or with this warning, so was
curious why does the warning is such a problem for the kernel. I'm sorry
I took your time and haven't got an answer that I entirely agree
with. Thank you very much for your explanations anyway.

> The warning is CRAP. End of story.

Amen!

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

"Pekka Enberg" <[EMAIL PROTECTED]> writes:

> On 2/13/07, Sergei Organov <[EMAIL PROTECTED]> wrote:
>> May I suggest another definition for a warning being entirely sucks?
>> "The warning is entirely sucks if and only if it never has true
>> positives." In all other cases it's only more or less sucks, IMHO.
>
> You're totally missing the point.

No, I don't. 

> False positives are not a minor annoyance, they're actively harmful as
> they hide other _useful_ warnings.

Every warning I'm aware of do have false positives. They are indeed
harmful, so one takes steps to get rid of them. If a warning had none
false positives, it should be an error, not a warning in the first
place.

> So, you really want warnings to be about things that can and
> should be fixed.

That's the definition of a "perfect" warning, that is actually called
"error".

> So you really should aim for _zero false positives_ even if you risk
> not detecting some real positives.

With almost any warning out there one makes more or less efforts to
suppress the warning where it gives false positives, isn't it? At least
that's my experience so far.

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Linus Torvalds <[EMAIL PROTECTED]> writes:
> On Mon, 12 Feb 2007, Sergei Organov wrote:
>> 
>> Why strlen() should be allowed to be called with an incompatible pointer
>> type? My point is that gcc should issue *different warning*, -- the same
>> warning it issues here:
>
> I agree that "strlen()" per se isn't different.

I agree that making strxxx() family special is not a good idea. So what
do we do for a random foo(char*) called with an 'unsigned char*'
argument? Silence? Hmmm... It's not immediately obvious that it's indeed
harmless. Yet another -Wxxx option to GCC to silence this particular
case?

> A warnign is only as good as
>  (a) the thing it warns about
>  (b) the thing you can do about it
> 
> And THAT is the fundamental problem with that *idiotic* warning. Yes, it's 
> technically correct. Yes, it's "proper C grammar". But if you can't get 
> over the hump of realizing that there is a difference between "grammar" 
> and "intelligent speech", you shouldn't be doing compilers.
> 
> So the warning sucks because:

May I suggest another definition for a warning being entirely sucks?
"The warning is entirely sucks if and only if it never has true
positives." In all other cases it's only more or less sucks, IMHO.

>  - the thing it warns about (passing "unsigned char" to something that 
>doesn't specify a sign at all!) is not something that sounds wrong in 
>the first place. Yes, it's unsigned. But no, the thing it is passed to 
>didn't specify that it wanted a "signed" thing in the first place. The 
>"strlen()" function literally says
>
>   "I want a char of indeterminate sign"!

I'm afraid I don't follow. Do we have a way to say "I want an int of
indeterminate sign" in C? The same way there doesn't seem to be a way
to say "I want a char of indeterminate sign". :( So no, strlen() doesn't
actually say that, no matter if we like it or not. It actually says "I
want a char with implementation-defined sign".

>which implies that strlen really doesn't care about the sign. The same 
>is true of *any* function that takes a "char *". Such a function 
>doesn't care, and fundamentally CANNOT care about the sign, since it's 
>not even defined!

In fact it's implementation-defined, and this may make a difference
here. strlen(), being part of C library, could be specifically
implemented for given architecture, and as architecture is free to
define the sign of "char", strlen() could in theory rely on particular
sign of "char" as defined for given architecture. [Not that I think that
any strlen() implementation actually depends on sign.]

>So the warning fails the (a) criterion. The warning isn't valid, 
>because the thing it warns about isn't a valid problem!

Can we assure that no function taking 'char*' ever cares about the sign?
I'm not sure, and I'm not a language lawyer, but if it's indeed the
case, I'd probably agree that it might be a good idea for GCC to extend
the C language so that function argument declared "char*" means either
of "char*", "signed char*", or "unsigned char*" even though there is no
precedent in the language.

BTW, the next logical step would be for "int*" argument to stop meaning
"signed int*" and become any of "int*", "signed int*" or "unsigned
int*". Isn't it cool to be able to declare a function that won't produce
warning no matter what int is passed to it? ;)

>  - it _also_ fails the (b) criterion, because quite often there is nothing 
>you can do about it. Yes, you can add a cast, but adding a cast 
>actually causes _worse_ code (but the warning is certainly gone). But 
>that makes the _other_ argument for the warning totally point-less: if 
>the reason for the warning was "bad code", then having the warning is 
>actively BAD, because the end result is actually "worse code".
>
> See? The second point is why it's important to also realize that there is 
> a lot of real and valid code that actually _does_ pass "strlen()" an 
> unsigned string.

And here we finally get to the practice...

> There are tons of reasons for that to happen: the part of the program
> that _does_ care wants to use a "unsigned char" array, because it ends
> up doing things like "isspace(array[x])", and that is not well-defined
> if you use a "char *" array.

Yes, indeed. So the real problem of the C language is inconsistency
between strxxx() and isxxx() families of functions? If so, what is 
wrong with actually fixing the problem, say, by using wrappers over
isxxx()? Checking... The kernel already 

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Linus Torvalds [EMAIL PROTECTED] writes:
 On Mon, 12 Feb 2007, Sergei Organov wrote:
 
 Why strlen() should be allowed to be called with an incompatible pointer
 type? My point is that gcc should issue *different warning*, -- the same
 warning it issues here:

 I agree that strlen() per se isn't different.

I agree that making strxxx() family special is not a good idea. So what
do we do for a random foo(char*) called with an 'unsigned char*'
argument? Silence? Hmmm... It's not immediately obvious that it's indeed
harmless. Yet another -Wxxx option to GCC to silence this particular
case?

 A warnign is only as good as
  (a) the thing it warns about
  (b) the thing you can do about it
 
 And THAT is the fundamental problem with that *idiotic* warning. Yes, it's 
 technically correct. Yes, it's proper C grammar. But if you can't get 
 over the hump of realizing that there is a difference between grammar 
 and intelligent speech, you shouldn't be doing compilers.
 
 So the warning sucks because:

May I suggest another definition for a warning being entirely sucks?
The warning is entirely sucks if and only if it never has true
positives. In all other cases it's only more or less sucks, IMHO.

  - the thing it warns about (passing unsigned char to something that 
doesn't specify a sign at all!) is not something that sounds wrong in 
the first place. Yes, it's unsigned. But no, the thing it is passed to 
didn't specify that it wanted a signed thing in the first place. The 
strlen() function literally says

   I want a char of indeterminate sign!

I'm afraid I don't follow. Do we have a way to say I want an int of
indeterminate sign in C? The same way there doesn't seem to be a way
to say I want a char of indeterminate sign. :( So no, strlen() doesn't
actually say that, no matter if we like it or not. It actually says I
want a char with implementation-defined sign.

which implies that strlen really doesn't care about the sign. The same 
is true of *any* function that takes a char *. Such a function 
doesn't care, and fundamentally CANNOT care about the sign, since it's 
not even defined!

In fact it's implementation-defined, and this may make a difference
here. strlen(), being part of C library, could be specifically
implemented for given architecture, and as architecture is free to
define the sign of char, strlen() could in theory rely on particular
sign of char as defined for given architecture. [Not that I think that
any strlen() implementation actually depends on sign.]

So the warning fails the (a) criterion. The warning isn't valid, 
because the thing it warns about isn't a valid problem!

Can we assure that no function taking 'char*' ever cares about the sign?
I'm not sure, and I'm not a language lawyer, but if it's indeed the
case, I'd probably agree that it might be a good idea for GCC to extend
the C language so that function argument declared char* means either
of char*, signed char*, or unsigned char* even though there is no
precedent in the language.

BTW, the next logical step would be for int* argument to stop meaning
signed int* and become any of int*, signed int* or unsigned
int*. Isn't it cool to be able to declare a function that won't produce
warning no matter what int is passed to it? ;)

  - it _also_ fails the (b) criterion, because quite often there is nothing 
you can do about it. Yes, you can add a cast, but adding a cast 
actually causes _worse_ code (but the warning is certainly gone). But 
that makes the _other_ argument for the warning totally point-less: if 
the reason for the warning was bad code, then having the warning is 
actively BAD, because the end result is actually worse code.

 See? The second point is why it's important to also realize that there is 
 a lot of real and valid code that actually _does_ pass strlen() an 
 unsigned string.

And here we finally get to the practice...

 There are tons of reasons for that to happen: the part of the program
 that _does_ care wants to use a unsigned char array, because it ends
 up doing things like isspace(array[x]), and that is not well-defined
 if you use a char * array.

Yes, indeed. So the real problem of the C language is inconsistency
between strxxx() and isxxx() families of functions? If so, what is 
wrong with actually fixing the problem, say, by using wrappers over
isxxx()? Checking... The kernel already uses isxxx() that are macros
that do conversion to unsigned char themselves, and a few invocations
of isspace() I've checked pass char as argument. So that's not a real
problem for the kernel, right?

 So there are lots of reasons to use unsigned char arrays for strings. 
 Look it up. Look up any half-way reasonable man-page for the isspace() 
 kind of functions, and if they don't actually explicitly say that you 
 should use unsigned characters for it, those man-pages are crap.

Well, even C99 itself does say it. But the kernel already handles this
issue

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Pekka Enberg [EMAIL PROTECTED] writes:

 On 2/13/07, Sergei Organov [EMAIL PROTECTED] wrote:
 May I suggest another definition for a warning being entirely sucks?
 The warning is entirely sucks if and only if it never has true
 positives. In all other cases it's only more or less sucks, IMHO.

 You're totally missing the point.

No, I don't. 

 False positives are not a minor annoyance, they're actively harmful as
 they hide other _useful_ warnings.

Every warning I'm aware of do have false positives. They are indeed
harmful, so one takes steps to get rid of them. If a warning had none
false positives, it should be an error, not a warning in the first
place.

 So, you really want warnings to be about things that can and
 should be fixed.

That's the definition of a perfect warning, that is actually called
error.

 So you really should aim for _zero false positives_ even if you risk
 not detecting some real positives.

With almost any warning out there one makes more or less efforts to
suppress the warning where it gives false positives, isn't it? At least
that's my experience so far.

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Linus Torvalds [EMAIL PROTECTED] writes:
 On Tue, 13 Feb 2007, Sergei Organov wrote:
 
 I want a char of indeterminate sign!
 
 I'm afraid I don't follow. Do we have a way to say I want an int of
 indeterminate sign in C? The same way there doesn't seem to be a way
 to say I want a char of indeterminate sign.

 You're wrong.

Sure, I knew it from the very beginning ;)


 Exactly because char *by*definition* is indeterminate sign as far as 
 something like strlen() is concerned.

Thanks, I now understand that you either don't see the difference
between indeterminate and implementation-defined in this context or
consider it non-essential, so I think I've got your point.

 char is _special_. Char is _different_. Char is *not* int.

Sure.


 So no, strlen() doesn't actually say that, no matter if we like it or 
 not. It actually says I want a char with implementation-defined sign.

 You're arguing some strange semantic difference in the English
 language.

Didn't I further explain what I meant exactly (that you've skipped)?
OK, never mind.

 I'm not really interested.

OK, no problem.


 THE FACT IS, THAT strlen() IS DEFINED UNIVERSALLY AS TAKING char *.

So just don't pass it unsigned char*. End of story.


 That BY DEFINITION means that strlen() cannot care about the sign, 
 because the sign IS NOT DEFINED UNIVERSALLY!

 And if you cannot accept that fact, it's your problem. Not mine.

I never had problems either with strlen() or with this warning, so was
curious why does the warning is such a problem for the kernel. I'm sorry
I took your time and haven't got an answer that I entirely agree
with. Thank you very much for your explanations anyway.

 The warning is CRAP. End of story.

Amen!

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Pekka Enberg [EMAIL PROTECTED] writes:
 On 2/13/07, Sergei Organov [EMAIL PROTECTED] wrote:
 With almost any warning out there one makes more or less efforts to
 suppress the warning where it gives false positives, isn't it?

 Yes, as long it's the _compiler_ that's doing the effort.
 You shouldn't need to annotate the source just to make the compiler
 shut up.

Sorry, what do you do with variable 'xxx' might be used uninitialized
warning when it's false? Turn it off? Annotate the source? Assign fake
initialization value? Change the compiler so that it does the effort
for you? Never encountered false positive from this warning?

 Once the compiler starts issuing enough false positives, it's
 time to turn off that warning completely.

Yes, I don't argue that. I said otherwise the warning is more or less
sucks, and then it's up to programmers to decide if it's enough sucks
to be turned off. The decision depends on the importance of its true
positives then. Only if warning never has true positives it is
unconditional, total, unhelpful crap, -- that was my point.

 Therefore, the only sane strategy for a warning is to aim for zero
 false positives.

Sure. But unfortunately this in an unreachable aim in most cases.

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Linus Torvalds <[EMAIL PROTECTED]> writes:
> On Fri, 9 Feb 2007, Sergei Organov wrote:
>> 
>> As far as I can read the C99 standard, the "char", "signed char", and
>> "unsigned char", are all different types:
>
> Indeed. Search for "pseudo-unsigned", and you'll see more. There are 
> actually cases where "char" can act differently from _both_ "unsigned 
> char" and "signed char".
>
>> If so, then the code above is broken no matter what representation of
>> "char" is chosen for given arch, as strcmp() takes "char*" arguments,
>> that are not compatible with either "signed char*" or "unsigned char*".
>
> ..and my argument is that a warning which doesn't allow you to call 
> "strlen()" on a "unsigned char" array without triggering is a bogus 
> warning, and must be removed.

Why strlen() should be allowed to be called with an incompatible pointer
type? My point is that gcc should issue *different warning*, -- the same
warning it issues here:

$ cat incompat.c
void foo(int *p);
void boo(long *p) { foo(p); }
$ gcc -W -Wall -c incompat.c
incompat.c:2: warning: passing argument 1 of 'foo' from incompatible pointer 
type

Calling strlen(char*) with "unsigned char*" argument does pass argument
of incompatible pointer type due to the fact that in C "char" and
"unsigned char" are two incompatible types, and it has nothing to do
with signedness.

[...]

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Linus Torvalds [EMAIL PROTECTED] writes:
 On Fri, 9 Feb 2007, Sergei Organov wrote:
 
 As far as I can read the C99 standard, the char, signed char, and
 unsigned char, are all different types:

 Indeed. Search for pseudo-unsigned, and you'll see more. There are 
 actually cases where char can act differently from _both_ unsigned 
 char and signed char.

 If so, then the code above is broken no matter what representation of
 char is chosen for given arch, as strcmp() takes char* arguments,
 that are not compatible with either signed char* or unsigned char*.

 ..and my argument is that a warning which doesn't allow you to call 
 strlen() on a unsigned char array without triggering is a bogus 
 warning, and must be removed.

Why strlen() should be allowed to be called with an incompatible pointer
type? My point is that gcc should issue *different warning*, -- the same
warning it issues here:

$ cat incompat.c
void foo(int *p);
void boo(long *p) { foo(p); }
$ gcc -W -Wall -c incompat.c
incompat.c:2: warning: passing argument 1 of 'foo' from incompatible pointer 
type

Calling strlen(char*) with unsigned char* argument does pass argument
of incompatible pointer type due to the fact that in C char and
unsigned char are two incompatible types, and it has nothing to do
with signedness.

[...]

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Jan Engelhardt <[EMAIL PROTECTED]> writes:
> On Feb 8 2007 08:33, Linus Torvalds wrote:
>>
[...]
> What C needs is a distinction between char and int8_t, rendering "char" 
> an unsigned at all times basically and making "unsigned char" and 
> "signed char" illegal types in turn.

AFAIK, C already has 3 *distinct* types, "char", "signed char", and
"unsigned char". So your int8_t reads "signed char" in C, uint8_t reads
"unsigned char" in C, while "char" is yet another type that is not
compatible with those two. Yes, the names for these types are somewhat
confusing, but that's due to historical reasons, I think.

Overall,

signed char   == tiny signed int   == tiny int
unsigned char == tiny unsigned int
char != signed char
char != unsigned char

where

tiny == short short ;)

Rules of thumb: don't use bare char to store integers; don't use
signed/unsigned char to store characters.

[strxxx() routines take char* arguments as they work on '\0'-terminated
character arrays (strings). Using them on arrays of tiny integers is
unsafe no matter how char behaves on given platform (like signed or like
unsigned type).]

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Linus Torvalds <[EMAIL PROTECTED]> writes:
> On Thu, 8 Feb 2007, Linus Torvalds wrote:
>> 
>> But THE CALLER CANNOT AND MUST NOT CARE! Because the sign of "char" is 
>> implementation-defined, so if you call "strcmp()", you are already 
>> basically saying: I don't care (and I _cannot_ care) what sign you are 
>> using.
>
> Let me explain it another way.
>
> Say you use
>
>   signed char *myname, *yourname;
>
>   if (strcmp(myname,yourname) < 0)
>   printf("Ha, I win!\n")
>
> and you compile this on an architecture where "char" is signed even 
> without the explicit "signed".

As far as I can read the C99 standard, the "char", "signed char", and
"unsigned char", are all different types:

 "The three types char, signed char, and unsigned char are collectively called
  the character types. The implementation shall define char to have the same 
range,
  representation, and behavior as either signed char or unsigned
  char.(35)
  
  (35) CHAR_MIN, defined in , will have one of the values 0 or
   SCHAR_MIN, and this can be used to distinguish the two
   options. Irrespective of the choice made, char is a separate type

   from the other two and is not compatible with either.
   ^
 "

If so, then the code above is broken no matter what representation of
"char" is chosen for given arch, as strcmp() takes "char*" arguments,
that are not compatible with either "signed char*" or "unsigned char*".

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Linus Torvalds [EMAIL PROTECTED] writes:
 On Thu, 8 Feb 2007, Linus Torvalds wrote:
 
 But THE CALLER CANNOT AND MUST NOT CARE! Because the sign of char is 
 implementation-defined, so if you call strcmp(), you are already 
 basically saying: I don't care (and I _cannot_ care) what sign you are 
 using.

 Let me explain it another way.

 Say you use

   signed char *myname, *yourname;

   if (strcmp(myname,yourname)  0)
   printf(Ha, I win!\n)

 and you compile this on an architecture where char is signed even 
 without the explicit signed.

As far as I can read the C99 standard, the char, signed char, and
unsigned char, are all different types:

 The three types char, signed char, and unsigned char are collectively called
  the character types. The implementation shall define char to have the same 
range,
  representation, and behavior as either signed char or unsigned
  char.(35)
  
  (35) CHAR_MIN, defined in limits.h, will have one of the values 0 or
   SCHAR_MIN, and this can be used to distinguish the two
   options. Irrespective of the choice made, char is a separate type

   from the other two and is not compatible with either.
   ^
 

If so, then the code above is broken no matter what representation of
char is chosen for given arch, as strcmp() takes char* arguments,
that are not compatible with either signed char* or unsigned char*.

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: somebody dropped a (warning) bomb

[Sergei Organov]

Jan Engelhardt [EMAIL PROTECTED] writes:
 On Feb 8 2007 08:33, Linus Torvalds wrote:

[...]
 What C needs is a distinction between char and int8_t, rendering char 
 an unsigned at all times basically and making unsigned char and 
 signed char illegal types in turn.

AFAIK, C already has 3 *distinct* types, char, signed char, and
unsigned char. So your int8_t reads signed char in C, uint8_t reads
unsigned char in C, while char is yet another type that is not
compatible with those two. Yes, the names for these types are somewhat
confusing, but that's due to historical reasons, I think.

Overall,

signed char   == tiny signed int   == tiny int
unsigned char == tiny unsigned int
char != signed char
char != unsigned char

where

tiny == short short ;)

Rules of thumb: don't use bare char to store integers; don't use
signed/unsigned char to store characters.

[strxxx() routines take char* arguments as they work on '\0'-terminated
character arrays (strings). Using them on arrays of tiny integers is
unsafe no matter how char behaves on given platform (like signed or like
unsigned type).]

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] serial: Add PCMCIA IDs for Quatech DSP-100 dual RS232 adapter.

[Sergei Organov]

Add PCMCIA IDs for Quatech DSP-100 dual RS232 adapter.

Signed-off-by: Sergei Organov <[EMAIL PROTECTED]>

---
diff --git a/drivers/serial/serial_cs.c b/drivers/serial/serial_cs.c
index 431433f..5757442 100644
--- a/drivers/serial/serial_cs.c
+++ b/drivers/serial/serial_cs.c
@@ -249,6 +249,10 @@ static const struct serial_quirk quirks[] = {
.multi  = 2,
}, {
.manfid = MANFID_QUATECH,
+   .prodid = PRODID_QUATECH_DUAL_RS232_G,
+   .multi  = 2,
+   }, {
+   .manfid = MANFID_QUATECH,
.prodid = PRODID_QUATECH_QUAD_RS232,
.multi  = 4,
}, {
@@ -893,6 +897,7 @@ static struct pcmcia_device_id serial_ids[] = {
PCMCIA_DEVICE_PROD_ID12("OEM  ", "C288MX ", 0xb572d360, 
0xd2385b7a),
PCMCIA_DEVICE_PROD_ID12("PCMCIA   ", "C336MX ", 0x99bcafe9, 
0xaa25bcab),
PCMCIA_DEVICE_PROD_ID12("Quatech Inc", "PCMCIA Dual RS-232 Serial Port 
Card", 0xc4420b35, 0x92abc92f),
+   PCMCIA_DEVICE_PROD_ID12("Quatech Inc", "Dual RS-232 Serial Port PC 
Card", 0xc4420b35, 0x031a380d),
PCMCIA_PFC_DEVICE_CIS_PROD_ID12(1, "PCMCIA", "EN2218-LAN/MODEM", 
0x281f1c5d, 0x570f348e, "PCMLM28.cis"),
PCMCIA_PFC_DEVICE_CIS_PROD_ID12(1, "PCMCIA", "UE2218-LAN/MODEM", 
0x281f1c5d, 0x6fdcacee, "PCMLM28.cis"),
PCMCIA_PFC_DEVICE_CIS_PROD_ID12(1, "Psion Dacom", "Gold Card V34 
Ethernet", 0xf5f025c2, 0x338e8155, "PCMLM28.cis"),
diff --git a/include/pcmcia/ciscode.h b/include/pcmcia/ciscode.h
index c1da855..eae7e2e 100644
--- a/include/pcmcia/ciscode.h
+++ b/include/pcmcia/ciscode.h
@@ -95,6 +95,7 @@
 #define PRODID_QUATECH_DUAL_RS232  0x0012
 #define PRODID_QUATECH_DUAL_RS232_D1   0x0007
 #define PRODID_QUATECH_DUAL_RS232_D2   0x0052
+#define PRODID_QUATECH_DUAL_RS232_G0x004d
 #define PRODID_QUATECH_QUAD_RS232  0x001b
 #define PRODID_QUATECH_DUAL_RS422  0x000e
 #define PRODID_QUATECH_QUAD_RS422  0x0045
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] serial: Add PCMCIA IDs for Quatech DSP-100 dual RS232 adapter.

[Sergei Organov]

Add PCMCIA IDs for Quatech DSP-100 dual RS232 adapter.

Signed-off-by: Sergei Organov [EMAIL PROTECTED]

---
diff --git a/drivers/serial/serial_cs.c b/drivers/serial/serial_cs.c
index 431433f..5757442 100644
--- a/drivers/serial/serial_cs.c
+++ b/drivers/serial/serial_cs.c
@@ -249,6 +249,10 @@ static const struct serial_quirk quirks[] = {
.multi  = 2,
}, {
.manfid = MANFID_QUATECH,
+   .prodid = PRODID_QUATECH_DUAL_RS232_G,
+   .multi  = 2,
+   }, {
+   .manfid = MANFID_QUATECH,
.prodid = PRODID_QUATECH_QUAD_RS232,
.multi  = 4,
}, {
@@ -893,6 +897,7 @@ static struct pcmcia_device_id serial_ids[] = {
PCMCIA_DEVICE_PROD_ID12(OEM  , C288MX , 0xb572d360, 
0xd2385b7a),
PCMCIA_DEVICE_PROD_ID12(PCMCIA   , C336MX , 0x99bcafe9, 
0xaa25bcab),
PCMCIA_DEVICE_PROD_ID12(Quatech Inc, PCMCIA Dual RS-232 Serial Port 
Card, 0xc4420b35, 0x92abc92f),
+   PCMCIA_DEVICE_PROD_ID12(Quatech Inc, Dual RS-232 Serial Port PC 
Card, 0xc4420b35, 0x031a380d),
PCMCIA_PFC_DEVICE_CIS_PROD_ID12(1, PCMCIA, EN2218-LAN/MODEM, 
0x281f1c5d, 0x570f348e, PCMLM28.cis),
PCMCIA_PFC_DEVICE_CIS_PROD_ID12(1, PCMCIA, UE2218-LAN/MODEM, 
0x281f1c5d, 0x6fdcacee, PCMLM28.cis),
PCMCIA_PFC_DEVICE_CIS_PROD_ID12(1, Psion Dacom, Gold Card V34 
Ethernet, 0xf5f025c2, 0x338e8155, PCMLM28.cis),
diff --git a/include/pcmcia/ciscode.h b/include/pcmcia/ciscode.h
index c1da855..eae7e2e 100644
--- a/include/pcmcia/ciscode.h
+++ b/include/pcmcia/ciscode.h
@@ -95,6 +95,7 @@
 #define PRODID_QUATECH_DUAL_RS232  0x0012
 #define PRODID_QUATECH_DUAL_RS232_D1   0x0007
 #define PRODID_QUATECH_DUAL_RS232_D2   0x0052
+#define PRODID_QUATECH_DUAL_RS232_G0x004d
 #define PRODID_QUATECH_QUAD_RS232  0x001b
 #define PRODID_QUATECH_DUAL_RS422  0x000e
 #define PRODID_QUATECH_QUAD_RS422  0x0045
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: QUATECH driver (was Free Linux Driver Development!)

[Sergei Organov]

Alan <[EMAIL PROTECTED]> writes:
>> 3. Vendor driver is rather close to the generic one being in the kernel,
>>so maybe it's better to improve generic one instead of adding yet
>>another driver to the tree.
>
> Firstly can you post a patch which adds the relevant identifiers to the
> current pcmcia serial driver so that 115,200 works but nothing higher.

OK, I'll try to recover what I did (as I've dropped the change after I
saw it can't do 460,800), and submit the patch.

> After that I'd like to take a look at the needed changes for higher speed
> support using the 2.6.20-mm work which adds arbitary speed support.

I expect it'll be rather hard for me to test the changes then, as my IBM
ThinkPad T43 suffers from kernel IDE/SATA/PATA issues, and running every
new kernel was a big pain so far (it currently runs 2.6.16.8) :(

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Free Linux Driver Development!

[Sergei Organov]

Greg KH <[EMAIL PROTECTED]> writes:

> On Wed, Jan 31, 2007 at 08:41:03PM +0300, Sergei Organov wrote:
>> Greg KH <[EMAIL PROTECTED]> writes:
>> [...]
>> >> And there are plenty of documented devices that no one cares enough
>> >> about to submit a driver for.
>> >
>> > Any specific examples?  I have a long list of people who wish to write
>> > new drivers but just don't know which hardware is not yet supported.
>> 
>> Maybe not entirely a new driver, as there already exist out-of-kernel
>> vendor GPL driver, but if somebody is willing to resolve the issue
>> described here:
>> 
>> <http://article.gmane.org/gmane.linux.serial/1221>
>> 
>> please contact me, and I'll be willing to help in testing as I have the
>> hardware.
>
> Do you have a pointer to the driver source anywhere?

Sure, here it is:

<http://www.quatech.com/ManualsDriversFirmware/Communication/serial-qtpcmcia-1.23.tar.gz>

> I suggest just posting it as a patch to the tree as documented in
> Documentation/SubmittingPatches and seeing how that goes.

That won't go, I'm afraid:

1. Vendor driver doesn't compile for recent kernels. I did compile it,
   but using brute-force approach that is not appropriate for the
   kernel.

2. There is generic driver in the kernel that supports this card among
   others, should I add the ID of this particular card, but doesn't
   support baud rates higher than 115200.

3. Vendor driver is rather close to the generic one being in the kernel,
   so maybe it's better to improve generic one instead of adding yet
   another driver to the tree.

-- Sergei Organov.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Free Linux Driver Development!

[Sergei Organov]

Greg KH [EMAIL PROTECTED] writes:

 On Wed, Jan 31, 2007 at 08:41:03PM +0300, Sergei Organov wrote:
 Greg KH [EMAIL PROTECTED] writes:
 [...]
  And there are plenty of documented devices that no one cares enough
  about to submit a driver for.
 
  Any specific examples?  I have a long list of people who wish to write
  new drivers but just don't know which hardware is not yet supported.
 
 Maybe not entirely a new driver, as there already exist out-of-kernel
 vendor GPL driver, but if somebody is willing to resolve the issue
 described here:
 
 http://article.gmane.org/gmane.linux.serial/1221
 
 please contact me, and I'll be willing to help in testing as I have the
 hardware.

 Do you have a pointer to the driver source anywhere?

Sure, here it is:

http://www.quatech.com/ManualsDriversFirmware/Communication/serial-qtpcmcia-1.23.tar.gz

 I suggest just posting it as a patch to the tree as documented in
 Documentation/SubmittingPatches and seeing how that goes.

That won't go, I'm afraid:

1. Vendor driver doesn't compile for recent kernels. I did compile it,
   but using brute-force approach that is not appropriate for the
   kernel.

2. There is generic driver in the kernel that supports this card among
   others, should I add the ID of this particular card, but doesn't
   support baud rates higher than 115200.

3. Vendor driver is rather close to the generic one being in the kernel,
   so maybe it's better to improve generic one instead of adding yet
   another driver to the tree.

-- Sergei Organov.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: QUATECH driver (was Free Linux Driver Development!)

[Sergei Organov]

Alan [EMAIL PROTECTED] writes:
 3. Vendor driver is rather close to the generic one being in the kernel,
so maybe it's better to improve generic one instead of adding yet
another driver to the tree.

 Firstly can you post a patch which adds the relevant identifiers to the
 current pcmcia serial driver so that 115,200 works but nothing higher.

OK, I'll try to recover what I did (as I've dropped the change after I
saw it can't do 460,800), and submit the patch.

 After that I'd like to take a look at the needed changes for higher speed
 support using the 2.6.20-mm work which adds arbitary speed support.

I expect it'll be rather hard for me to test the changes then, as my IBM
ThinkPad T43 suffers from kernel IDE/SATA/PATA issues, and running every
new kernel was a big pain so far (it currently runs 2.6.16.8) :(

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Free Linux Driver Development!

[Sergei Organov]

Greg KH <[EMAIL PROTECTED]> writes:
[...]
>> And there are plenty of documented devices that no one cares enough
>> about to submit a driver for.
>
> Any specific examples?  I have a long list of people who wish to write
> new drivers but just don't know which hardware is not yet supported.

Maybe not entirely a new driver, as there already exist out-of-kernel
vendor GPL driver, but if somebody is willing to resolve the issue
described here:



please contact me, and I'll be willing to help in testing as I have the
hardware.

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Free Linux Driver Development!

[Sergei Organov]

Greg KH [EMAIL PROTECTED] writes:
[...]
 And there are plenty of documented devices that no one cares enough
 about to submit a driver for.

 Any specific examples?  I have a long list of people who wish to write
 new drivers but just don't know which hardware is not yet supported.

Maybe not entirely a new driver, as there already exist out-of-kernel
vendor GPL driver, but if somebody is willing to resolve the issue
described here:

http://article.gmane.org/gmane.linux.serial/1221

please contact me, and I'll be willing to help in testing as I have the
hardware.

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/4] Char: mxser_new, fix twice resource releasing

[Sergei Organov]

Jiri Slaby <[EMAIL PROTECTED]> writes:

> mxser_new, fix twice resource releasing
>

Hi Jiri,

I've noticed the patch(es) and will be happy to test them after the
holidays.

Thank you very much for working on these issues and Happy New Year!

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/4] Char: mxser_new, fix twice resource releasing

[Sergei Organov]

Jiri Slaby [EMAIL PROTECTED] writes:

 mxser_new, fix twice resource releasing


Hi Jiri,

I've noticed the patch(es) and will be happy to test them after the
holidays.

Thank you very much for working on these issues and Happy New Year!

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

irq 4: nobody cared and I/O errors on serial ports.

[Sergei Organov]

Hello,

It seems that the kernel has some problems/races in opening/closing of
serial ports. Simple C program below just opens/closes a port in a loop:

#include 
#include 
#include 
#include 

int main()
{
  while(1) {
int fd = open("/dev/ttyS0", O_RDONLY | O_NOCTTY);
if(fd < 0)
  fprintf(stderr, "%s\n", strerror(errno));
else
  close(fd);
  }
}

I've noticed 2 problems running this program. I run 2.6.19.1 smp kernel
(I've also tested Debian 2.6.18.3 kernel, and it has the same issues) on
hyper-threaded Pentium 4 CPU.

1. When I run the program, I begin to get "irq 4: nobody cared" in dmesg even
   though the port is not connected (idle). Please find relevant part of dmesg
   below.

2. When two copies of this program are run simultaneously, each of
   copies start to randomly fail to open the port with errno=5
   (Input/output error).

Note that I've tested this both with standard PC port ttyS0 and with
serial ports of MOXA multi-port serial board (ttyM*), and [mis]behavior
is the same. Also note that opening /dev/null instead of serial port
doesn't have any problems.

Here are relevant parts from dmesg when open/close ttyS0:

irq 4: nobody cared (try booting with the "irqpoll" option)
 [] __report_bad_irq+0x36/0x7d
 [] note_interrupt+0x1bb/0x1f7
 [] handle_IRQ_event+0x1a/0x3f
 [] handle_edge_irq+0xde/0x109
 [] do_IRQ+0x7d/0xa4
 [] common_interrupt+0x1a/0x20
 [] common_interrupt+0x1a/0x20
 [] serial_out+0x73/0x77
 [] serial8250_shutdown+0x71/0x148
 [] uart_shutdown+0x83/0xad
 [] uart_close+0x113/0x1a9
 [] tty_fasync+0x3a/0xaa
 [] release_dev+0x1df/0x61e
 [] chrdev_open+0x12d/0x141
 [] chrdev_open+0x0/0x141
 [] nameidata_to_filp+0x24/0x33
 [] do_filp_open+0x32/0x39
 [] __next_cpu+0x12/0x21
 [] __sched_text_start+0x5a3/0x90a
 [] tty_release+0xf/0x18
 [] __fput+0x96/0x16a
 [] filp_close+0x52/0x59
 [] sys_close+0x65/0x99
 [] sysenter_past_esp+0x56/0x79
 ===
handlers:
[] (serial8250_interrupt+0x0/0xdc)
Disabling IRQ #4
irq 4: nobody cared (try booting with the "irqpoll" option)
 [] __report_bad_irq+0x36/0x7d
 [] note_interrupt+0x1bb/0x1f7
 [] handle_IRQ_event+0x1a/0x3f
 [] handle_edge_irq+0xde/0x109
 [] do_IRQ+0x7d/0xa4
 [] common_interrupt+0x1a/0x20
 [] mwait_idle_with_hints+0x3b/0x3f
 [] mwait_idle+0xc/0x1b
 [] cpu_idle+0x9f/0xb9
 [] start_kernel+0x39f/0x3a7
 [] unknown_bootoption+0x0/0x206
 ===
handlers:
Disabling IRQ #4
irq 4: nobody cared (try booting with the "irqpoll" option)
 [] __report_bad_irq+0x36/0x7d
 [] note_interrupt+0x1bb/0x1f7
 [] handle_IRQ_event+0x1a/0x3f
 [] handle_edge_irq+0xde/0x109
 [] do_IRQ+0x7d/0xa4
 [] common_interrupt+0x1a/0x20
 [] mwait_idle_with_hints+0x3b/0x3f
 [] mwait_idle+0xc/0x1b
 [] cpu_idle+0x9f/0xb9
 [] start_kernel+0x39f/0x3a7
 [] unknown_bootoption+0x0/0x206
 ===
handlers:
Disabling IRQ #4
irq 4: nobody cared (try booting with the "irqpoll" option)
 [] __report_bad_irq+0x36/0x7d
 [] note_interrupt+0x1bb/0x1f7
 [] handle_IRQ_event+0x1a/0x3f
 [] handle_edge_irq+0xde/0x109
 [] do_IRQ+0x7d/0xa4
 [] common_interrupt+0x1a/0x20
 [] common_interrupt+0x1a/0x20
 [] serial_out+0x73/0x77
 [] serial8250_shutdown+0x8b/0x148
 [] uart_shutdown+0x83/0xad
 [] uart_close+0x113/0x1a9
 [] tty_fasync+0x3a/0xaa
 [] release_dev+0x1df/0x61e
 [] chrdev_open+0x12d/0x141
 [] chrdev_open+0x0/0x141
 [] nameidata_to_filp+0x24/0x33
 [] do_filp_open+0x32/0x39
 [] __next_cpu+0x12/0x21
 [] __sched_text_start+0x5a3/0x90a
 [] tty_release+0xf/0x18
 [] __fput+0x96/0x16a
 [] filp_close+0x52/0x59
 [] sys_close+0x65/0x99
 [] sysenter_past_esp+0x56/0x79
 ===
handlers:
[] (serial8250_interrupt+0x0/0xdc)
Disabling IRQ #4
irq 4: nobody cared (try booting with the "irqpoll" option)
 [] __report_bad_irq+0x36/0x7d
 [] note_interrupt+0x1bb/0x1f7
 [] handle_IRQ_event+0x1a/0x3f
 [] handle_edge_irq+0xde/0x109
 [] do_IRQ+0x7d/0xa4
 [] common_interrupt+0x1a/0x20
 [] mwait_idle_with_hints+0x3b/0x3f
 [] mwait_idle+0xc/0x1b
 [] cpu_idle+0x9f/0xb9
 [] start_kernel+0x39f/0x3a7
 [] unknown_bootoption+0x0/0x206
 ===
handlers:
Disabling IRQ #4


-- 
Sergei Organov.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: moxa serial driver testing

[Sergei Organov]

Jiri Slaby <[EMAIL PROTECTED]> writes:

[...]

>> # rmmod mxser_new
>> Trying to free already-free IRQ 58
>> Trying to free nonexistent resource <9000-903f>
>> Trying to free nonexistent resource <8800-8800>
>
> Thanks, I'll fix this and let you know. Does this happed every time you try to
> unload it?

Yes, it's stable. Happens every time.

-- Sergei.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: moxa serial driver testing

[Sergei Organov]

Jiri Slaby [EMAIL PROTECTED] writes:

[...]

 # rmmod mxser_new
 Trying to free already-free IRQ 58
 Trying to free nonexistent resource 9000-903f
 Trying to free nonexistent resource 8800-8800

 Thanks, I'll fix this and let you know. Does this happed every time you try to
 unload it?

Yes, it's stable. Happens every time.

-- Sergei.

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

irq 4: nobody cared and I/O errors on serial ports.

[Sergei Organov]

Hello,

It seems that the kernel has some problems/races in opening/closing of
serial ports. Simple C program below just opens/closes a port in a loop:

#include stdio.h
#include unistd.h
#include errno.h
#include fcntl.h

int main()
{
  while(1) {
int fd = open(/dev/ttyS0, O_RDONLY | O_NOCTTY);
if(fd  0)
  fprintf(stderr, %s\n, strerror(errno));
else
  close(fd);
  }
}

I've noticed 2 problems running this program. I run 2.6.19.1 smp kernel
(I've also tested Debian 2.6.18.3 kernel, and it has the same issues) on
hyper-threaded Pentium 4 CPU.

1. When I run the program, I begin to get irq 4: nobody cared in dmesg even
   though the port is not connected (idle). Please find relevant part of dmesg
   below.

2. When two copies of this program are run simultaneously, each of
   copies start to randomly fail to open the port with errno=5
   (Input/output error).

Note that I've tested this both with standard PC port ttyS0 and with
serial ports of MOXA multi-port serial board (ttyM*), and [mis]behavior
is the same. Also note that opening /dev/null instead of serial port
doesn't have any problems.

Here are relevant parts from dmesg when open/close ttyS0:

irq 4: nobody cared (try booting with the irqpoll option)
 [c0144ef2] __report_bad_irq+0x36/0x7d
 [c01450f4] note_interrupt+0x1bb/0x1f7
 [c0144665] handle_IRQ_event+0x1a/0x3f
 [c01458fd] handle_edge_irq+0xde/0x109
 [c010537d] do_IRQ+0x7d/0xa4
 [c01036ee] common_interrupt+0x1a/0x20
 [c01036ee] common_interrupt+0x1a/0x20
 [c021b63e] serial_out+0x73/0x77
 [c021cbb1] serial8250_shutdown+0x71/0x148
 [c0219c57] uart_shutdown+0x83/0xad
 [c021b16b] uart_close+0x113/0x1a9
 [c0209bf9] tty_fasync+0x3a/0xaa
 [c0209e48] release_dev+0x1df/0x61e
 [c016286d] chrdev_open+0x12d/0x141
 [c0162740] chrdev_open+0x0/0x141
 [c015ed34] nameidata_to_filp+0x24/0x33
 [c015ed75] do_filp_open+0x32/0x39
 [c01c1fa4] __next_cpu+0x12/0x21
 [c02964a3] __sched_text_start+0x5a3/0x90a
 [c020a296] tty_release+0xf/0x18
 [c0160f29] __fput+0x96/0x16a
 [c015ea56] filp_close+0x52/0x59
 [c015fa2b] sys_close+0x65/0x99
 [c0102cfd] sysenter_past_esp+0x56/0x79
 ===
handlers:
[c021d9d4] (serial8250_interrupt+0x0/0xdc)
Disabling IRQ #4
irq 4: nobody cared (try booting with the irqpoll option)
 [c0144ef2] __report_bad_irq+0x36/0x7d
 [c01450f4] note_interrupt+0x1bb/0x1f7
 [c0144665] handle_IRQ_event+0x1a/0x3f
 [c01458fd] handle_edge_irq+0xde/0x109
 [c010537d] do_IRQ+0x7d/0xa4
 [c01036ee] common_interrupt+0x1a/0x20
 [c0101235] mwait_idle_with_hints+0x3b/0x3f
 [c0101245] mwait_idle+0xc/0x1b
 [c0101c7f] cpu_idle+0x9f/0xb9
 [c0333753] start_kernel+0x39f/0x3a7
 [c03331ae] unknown_bootoption+0x0/0x206
 ===
handlers:
Disabling IRQ #4
irq 4: nobody cared (try booting with the irqpoll option)
 [c0144ef2] __report_bad_irq+0x36/0x7d
 [c01450f4] note_interrupt+0x1bb/0x1f7
 [c0144665] handle_IRQ_event+0x1a/0x3f
 [c01458fd] handle_edge_irq+0xde/0x109
 [c010537d] do_IRQ+0x7d/0xa4
 [c01036ee] common_interrupt+0x1a/0x20
 [c0101235] mwait_idle_with_hints+0x3b/0x3f
 [c0101245] mwait_idle+0xc/0x1b
 [c0101c7f] cpu_idle+0x9f/0xb9
 [c0333753] start_kernel+0x39f/0x3a7
 [c03331ae] unknown_bootoption+0x0/0x206
 ===
handlers:
Disabling IRQ #4
irq 4: nobody cared (try booting with the irqpoll option)
 [c0144ef2] __report_bad_irq+0x36/0x7d
 [c01450f4] note_interrupt+0x1bb/0x1f7
 [c0144665] handle_IRQ_event+0x1a/0x3f
 [c01458fd] handle_edge_irq+0xde/0x109
 [c010537d] do_IRQ+0x7d/0xa4
 [c01036ee] common_interrupt+0x1a/0x20
 [c01036ee] common_interrupt+0x1a/0x20
 [c021b63e] serial_out+0x73/0x77
 [c021cbcb] serial8250_shutdown+0x8b/0x148
 [c0219c57] uart_shutdown+0x83/0xad
 [c021b16b] uart_close+0x113/0x1a9
 [c0209bf9] tty_fasync+0x3a/0xaa
 [c0209e48] release_dev+0x1df/0x61e
 [c016286d] chrdev_open+0x12d/0x141
 [c0162740] chrdev_open+0x0/0x141
 [c015ed34] nameidata_to_filp+0x24/0x33
 [c015ed75] do_filp_open+0x32/0x39
 [c01c1fa4] __next_cpu+0x12/0x21
 [c02964a3] __sched_text_start+0x5a3/0x90a
 [c020a296] tty_release+0xf/0x18
 [c0160f29] __fput+0x96/0x16a
 [c015ea56] filp_close+0x52/0x59
 [c015fa2b] sys_close+0x65/0x99
 [c0102cfd] sysenter_past_esp+0x56/0x79
 ===
handlers:
[c021d9d4] (serial8250_interrupt+0x0/0xdc)
Disabling IRQ #4
irq 4: nobody cared (try booting with the irqpoll option)
 [c0144ef2] __report_bad_irq+0x36/0x7d
 [c01450f4] note_interrupt+0x1bb/0x1f7
 [c0144665] handle_IRQ_event+0x1a/0x3f
 [c01458fd] handle_edge_irq+0xde/0x109
 [c010537d] do_IRQ+0x7d/0xa4
 [c01036ee] common_interrupt+0x1a/0x20
 [c0101235] mwait_idle_with_hints+0x3b/0x3f
 [c0101245] mwait_idle+0xc/0x1b
 [c0101c7f] cpu_idle+0x9f/0xb9
 [c0333753] start_kernel+0x39f/0x3a7
 [c03331ae] unknown_bootoption+0x0/0x206
 ===
handlers:
Disabling IRQ #4


-- 
Sergei Organov.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: moxa serial driver testing

[Sergei Organov]

Jiri Slaby <[EMAIL PROTECTED]> writes:

>> Jiri Slaby <[EMAIL PROTECTED]> writes:
>> 
>> > Could you test the patch below, if something changes?
>> 
>> Just tested with low_latency commented out. Still oopses:
>> 
>> BUG: unable to handle kernel NULL pointer dereference at virtual address 
>> 0008
>>  printing eip:
>> f8f1730f
>> *pde = 
>> Oops:  [#1]
>> SMP 
>> Modules linked in: ...
>> CPU:0
>> EIP:0060:[]Tainted: P  VLI
>> EFLAGS: 00010046   (2.6.18-3-686 #1) 
>> EIP is at mxser_receive_chars+0x21b/0x249 [mxser_new]
>
> Yes, port->tty still somewhere becomes NULL -- does this patch help?

Hi, Jiri!

I'm so sorry, I don't know what I smoked yesterday, but the latest
version you've sent me does not have this problem anymore! I think I
copied compiled module to modules directory for different kernel and
thus tested old code all the time. BTW, should you tell me that the
ports are now called /dev/ttyMIx instead of /dev/ttyMx, I think I'd
notice my mistake earlier. Yes, in fact I didn't test any version where
ports are called /dev/ttyMIx until now! In particular, it means that
maybe some of the recent changes you've made yesterday based on my wrong
reports are not in fact required.

Anyway, the only mxser-specific problem that currently remains for
me and that I didn't see before, is the following:

# rmmod mxser_new
Trying to free already-free IRQ 58
Trying to free nonexistent resource <9000-903f>
Trying to free nonexistent resource <8800-8800>
#

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: moxa serial driver testing

[Sergei Organov]

Jiri Slaby [EMAIL PROTECTED] writes:

 Jiri Slaby [EMAIL PROTECTED] writes:
 
  Could you test the patch below, if something changes?
 
 Just tested with low_latency commented out. Still oopses:
 
 BUG: unable to handle kernel NULL pointer dereference at virtual address 
 0008
  printing eip:
 f8f1730f
 *pde = 
 Oops:  [#1]
 SMP 
 Modules linked in: ...
 CPU:0
 EIP:0060:[f8f1730f]Tainted: P  VLI
 EFLAGS: 00010046   (2.6.18-3-686 #1) 
 EIP is at mxser_receive_chars+0x21b/0x249 [mxser_new]

 Yes, port-tty still somewhere becomes NULL -- does this patch help?

Hi, Jiri!

I'm so sorry, I don't know what I smoked yesterday, but the latest
version you've sent me does not have this problem anymore! I think I
copied compiled module to modules directory for different kernel and
thus tested old code all the time. BTW, should you tell me that the
ports are now called /dev/ttyMIx instead of /dev/ttyMx, I think I'd
notice my mistake earlier. Yes, in fact I didn't test any version where
ports are called /dev/ttyMIx until now! In particular, it means that
maybe some of the recent changes you've made yesterday based on my wrong
reports are not in fact required.

Anyway, the only mxser-specific problem that currently remains for
me and that I didn't see before, is the following:

# rmmod mxser_new
Trying to free already-free IRQ 58
Trying to free nonexistent resource 9000-903f
Trying to free nonexistent resource 8800-8800
#

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: moxa serial driver testing

[Sergei Organov]

Jiri Slaby <[EMAIL PROTECTED]> writes:

>> Jiri Slaby <[EMAIL PROTECTED]> writes:
>> 
>> > Could you test the patch below, if something changes?
>> 
>> Just tested with low_latency commented out. Still oopses:
>> 
>> BUG: unable to handle kernel NULL pointer dereference at virtual address 
>> 0008
>>  printing eip:
>> f8f1730f
>> *pde = 
>> Oops:  [#1]
>> SMP 
>> Modules linked in: nvidia agpgart ipv6 nfs lockd nfs_acl sunrpc dm_mod 
>> sr_mod sbp2 ieee1394 ide_generic ide_disk e1000 snd_hda_intel snd_hda_codec 
>> snd_pcm_oss snd_mixer_oss snd_pcm snd_timer i2c_i801 tsdev psmouse snd 
>> soundcore snd_page_alloc i2c_core serio_raw parport_pc parport mxser_new 
>> evdev floppy pcspkr rtc ext3 jbd mbcache usb_storage usbhid ide_cd cdrom 
>> sd_mod uhci_hcd piix usbcore skge ata_piix libata scsi_mod generic ide_core 
>> thermal processor fan
>> CPU:0
>> EIP:0060:[]Tainted: P  VLI
>> EFLAGS: 00010046   (2.6.18-3-686 #1) 
>> EIP is at mxser_receive_chars+0x21b/0x249 [mxser_new]
>
> Yes, port->tty still somewhere becomes NULL -- does this patch help?

In addition to my previous answer. I've performed the same tests with
regular PC serial port. The issues with "Disabling Irq #N" and with I/O
error on open() exist with this port as well, so they aren't
mxser-specific.  I wasn't able to reproduce oopses, so they do seem to
be mxser-specific.

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: moxa serial driver testing

[Sergei Organov]

Jiri Slaby <[EMAIL PROTECTED]> writes:

>> Jiri Slaby <[EMAIL PROTECTED]> writes:
>> 
>> > Could you test the patch below, if something changes?
>> 
>> Just tested with low_latency commented out. Still oopses:
>> 
>> BUG: unable to handle kernel NULL pointer dereference at virtual address 
>> 0008
>>  printing eip:
>> f8f1730f
>> *pde = 
>> Oops:  [#1]
>> SMP 
>> Modules linked in: nvidia agpgart ipv6 nfs lockd nfs_acl sunrpc dm_mod 
>> sr_mod sbp2 ieee1394 ide_generic ide_disk e1000 snd_hda_intel snd_hda_codec 
>> snd_pcm_oss snd_mixer_oss snd_pcm snd_timer i2c_i801 tsdev psmouse snd 
>> soundcore snd_page_alloc i2c_core serio_raw parport_pc parport mxser_new 
>> evdev floppy pcspkr rtc ext3 jbd mbcache usb_storage usbhid ide_cd cdrom 
>> sd_mod uhci_hcd piix usbcore skge ata_piix libata scsi_mod generic ide_core 
>> thermal processor fan
>> CPU:0
>> EIP:0060:[]Tainted: P  VLI
>> EFLAGS: 00010046   (2.6.18-3-686 #1) 
>> EIP is at mxser_receive_chars+0x21b/0x249 [mxser_new]
>
> Yes, port->tty still somewhere becomes NULL -- does this patch help?

No, it still oopses (the oops is at the end).

In addition, before this latest patch was applied, when playing with two
simultaneously run programs that do nothing but open/close the port in a
loop (and the port was idle, i.e., nothing came to it from outside),
I've once got this thing:

irq 58: nobody cared (try booting with the "irqpoll" option)
 [] __report_bad_irq+0x2b/0x69
 [] note_interrupt+0x1af/0x1e7
 [] handle_IRQ_event+0x23/0x49
 [] __do_IRQ+0xb3/0xe8
 [] do_IRQ+0x43/0x52
 [] common_interrupt+0x1a/0x20
 [] __cpu_up+0x88/0x164
 [] _spin_unlock_irqrestore+0x8/0x9
 [] mxser_startup+0x177/0x190 [mxser_new]
 [] mxser_open+0x6b/0x251 [mxser_new]
 [] tty_open+0x16a/0x2e8
 [] chrdev_open+0x126/0x141
 [] chrdev_open+0x0/0x141
 [] __dentry_open+0xc8/0x1ac
 [] nameidata_to_filp+0x19/0x28
 [] do_filp_open+0x2b/0x31
 [] do_sys_open+0x3e/0xb3
 [] sys_open+0x16/0x18
 [] sysenter_past_esp+0x56/0x79
handlers:
[] (mxser_interrupt+0x0/0x1e6 [mxser_new])
Disabling IRQ #58

Another thing that I've noticed, is that when two programs that
open/close a port in a loop run simultaneously, each of them begin to
sometimes fail to open the port with errno=5 (I/O Errror). Though this
thing happends even if I try to use /dev/ttyS0, so it is not
mxser-specific (/dev/null never fails).

Here is the oops I promised at the beginning:

BUG: unable to handle kernel NULL pointer dereference at virtual address 
0008
 printing eip:
f927630f
*pde = 
Oops:  [#1]
SMP 
Modules linked in: mxser_new nvidia agpgart ipv6 nfs lockd nfs_acl sunrpc 
dm_mod sr_mod sbp2 ieee1394 ide_generic ide_disk e1000 snd_hda_intel 
snd_hda_codec snd_pcm_oss snd_mixer_oss snd_pcm psmouse snd_timer i2c_i801 snd 
serio_raw i2c_core tsdev parport_pc parport pcspkr evdev rtc soundcore 
snd_page_alloc floppy ext3 jbd mbcache sd_mod usb_storage usbhid ide_cd cdrom 
ata_piix libata scsi_mod uhci_hcd piix generic skge usbcore ide_core thermal 
processor fan
CPU:0
EIP:0060:[]Tainted: P  VLI
EFLAGS: 00010046   (2.6.18-3-686 #1) 
EIP is at mxser_receive_chars+0x21b/0x249 [mxser_new]
eax:    ebx:    ecx:    edx: 0286
esi: f927b79c   edi: 0001   ebp: c1991800   esp: c0313efc
ds: 007b   es: 007b   ss: 0068
Process swapper (pid: 0, ti=c0312000 task=c02c76a0 task.ti=c0312000)
Stack: c0313f48 f927b8a8 903a  0fff 00ff 0286 007b 
   f927b79c 0006 007f 00c6 f9276d7a 0007 0008 0080 
     f927adc0 0060 dfc74980   003a 
Call Trace:
 [] mxser_interrupt+0x15f/0x1e6 [mxser_new]
 [] handle_IRQ_event+0x23/0x49
 [] __do_IRQ+0x93/0xe8
 [] do_IRQ+0x43/0x52
 [] common_interrupt+0x1a/0x20
 [] mwait_idle+0x25/0x38
 [] cpu_idle+0x9f/0xb9
 [] start_kernel+0x379/0x380
Code: ed ff ff eb 1f 8b 06 83 78 18 00 75 17 8b 56 08 83 c2 05 ec 8b 14 24 0f 
b6 c0 a8 01 89 02 0f 85 d4 fe ff ff 8b 46 04 8b 54 24 18 <8b> 40 08 01 3c 85 c4 
dc 27 f9 8b 44 24 04 01 be f4 00 00 00 01 
EIP: [] mxser_receive_chars+0x21b/0x249 [mxser_new] SS:ESP 
0068:c0313efc
 <0>Kernel panic - not syncing: Fatal exception in interrupt
 BUG: warning at arch/i386/kernel/smp.c:547/smp_call_function()
 [] smp_call_function+0x53/0xfe
 [] printk+0x14/0x18
 [] smp_send_stop+0x13/0x1c
 [] panic+0x4c/0xe2
 [] die+0x256/0x28a
 [] do_page_fault+0x3b4/0x481
 [] tty_buffer_request_room+0x107/0x112
 [] do_page_fault+0x0/0x481
 [] error_code+0x39/0x40
 [] mxser_receive_chars+0x21b/0x249 [mxser_new]
 [] mxser_interrupt+0x15f/0x1e6 [mxser_new]
 [] handle_IRQ_event+0x23/0x49
 [] __do_IRQ+0x93/0xe8
 [] do_IRQ+0x43/0x52
 [] common_interrupt+0x1a/0x20
 [] mwait_idle+0x25/0x38
 [] cpu_idle+0x9f/0xb9
 [] start_kernel+0x379/0x380
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please 

Re: moxa serial driver testing

[Sergei Organov]

Jiri Slaby <[EMAIL PROTECTED]> writes:

> [EMAIL PROTECTED] wrote:
>> Hi Jiri,
>> 
>> I've figured out that both old and new mxser drivers have two similar
>> problems:
>> 
>> 1. When there are data coming to a port, sometimes opening of the port
>>entirely locks the box. This is quite reproducible. Any idea what's
>>wrong and how can I help to debug it?
>
> Could you test the patch below, if something changes?

Just tested with low_latency commented out. Still oopses:

BUG: unable to handle kernel NULL pointer dereference at virtual address 
0008
 printing eip:
f8f1730f
*pde = 
Oops:  [#1]
SMP 
Modules linked in: nvidia agpgart ipv6 nfs lockd nfs_acl sunrpc dm_mod sr_mod 
sbp2 ieee1394 ide_generic ide_disk e1000 snd_hda_intel snd_hda_codec 
snd_pcm_oss snd_mixer_oss snd_pcm snd_timer i2c_i801 tsdev psmouse snd 
soundcore snd_page_alloc i2c_core serio_raw parport_pc parport mxser_new evdev 
floppy pcspkr rtc ext3 jbd mbcache usb_storage usbhid ide_cd cdrom sd_mod 
uhci_hcd piix usbcore skge ata_piix libata scsi_mod generic ide_core thermal 
processor fan
CPU:0
EIP:0060:[]Tainted: P  VLI
EFLAGS: 00010046   (2.6.18-3-686 #1) 
EIP is at mxser_receive_chars+0x21b/0x249 [mxser_new]
eax:    ebx:    ecx:    edx: 0286
esi: f8f1c79c   edi: 0001   ebp: c1be6000   esp: c0313efc
ds: 007b   es: 007b   ss: 0068
Process swapper (pid: 0, ti=c0312000 task=c02c76a0 task.ti=c0312000)
Stack: c0313f48 f8f1c8a8 903d  0fff 00ff 0286 007b 
   f8f1c79c 0006 007f 00c6 f8f17d7a 0007 0008 0080 
     f8f1bdc0 0060 df985500   003a 
Call Trace:
 [] mxser_interrupt+0x15f/0x1e6 [mxser_new]
 [] handle_IRQ_event+0x23/0x49
 [] __do_IRQ+0x93/0xe8
 [] do_IRQ+0x43/0x52
 [] common_interrupt+0x1a/0x20
 [] mwait_idle+0x25/0x38
 [] cpu_idle+0x9f/0xb9
 [] start_kernel+0x379/0x380
Code: ed ff ff eb 1f 8b 06 83 78 18 00 75 17 8b 56 08 83 c2 05 ec 8b 14 24 0f 
b6 c0 a8 01 89 02 0f 85 d4 fe ff ff 8b 46 04 8b 54 24 18 <8b> 40 08 01 3c 85 c4 
ec f1 f8 8b 44 24 04 01 be f4 00 00 00 01 
EIP: [] mxser_receive_chars+0x21b/0x249 [mxser_new] SS:ESP 
0068:c0313efc
 <0>Kernel panic - not syncing: Fatal exception in interrupt
 BUG: warning at arch/i386/kernel/smp.c:547/smp_call_function()
 [] smp_call_function+0x53/0xfe
 [] printk+0x14/0x18
 [] smp_send_stop+0x13/0x1c
 [] panic+0x4c/0xe2
 [] die+0x256/0x28a
 [] do_page_fault+0x3b4/0x481
 [] tty_buffer_request_room+0x107/0x112
 [] do_page_fault+0x0/0x481
 [] error_code+0x39/0x40
 [] mxser_receive_chars+0x21b/0x249 [mxser_new]
 [] mxser_interrupt+0x15f/0x1e6 [mxser_new]
 [] handle_IRQ_event+0x23/0x49
 [] __do_IRQ+0x93/0xe8
 [] do_IRQ+0x43/0x52
 [] common_interrupt+0x1a/0x20
 [] mwait_idle+0x25/0x38
 [] cpu_idle+0x9f/0xb9
 [] start_kernel+0x379/0x380
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: moxa serial driver testing

[Sergei Organov]

Jiri Slaby <[EMAIL PROTECTED]> writes:
> [EMAIL PROTECTED] wrote:
>> Hi Jiri,
>> 
>> I've figured out that both old and new mxser drivers have two similar
>> problems:
>> 
>> 1. When there are data coming to a port, sometimes opening of the port
>>entirely locks the box. This is quite reproducible. Any idea what's
>>wrong and how can I help to debug it?
>
> Could you test the patch below, if something changes?

I've tested the latest version you've sent me yesterday. Still no
luck. The oops is below. I'll try with low_latency commented in a few
minutes.

BUG: unable to handle kernel NULL pointer dereference at virtual address 
0068
 printing eip:
f8f3711f
*pde = 
Oops:  [#1]
SMP 
Modules linked in: nvidia agpgart ipv6 nfs lockd nfs_acl sunrpc dm_mod sr_mod 
sbp2 ieee1394 ide_generic ide_disk e1000 snd_hda_intel snd_hda_codec 
snd_pcm_oss snd_mixer_oss i2c_i801 psmouse snd_pcm floppy tsdev serio_raw 
snd_timer parport_pc parport snd i2c_core pcspkr evdev soundcore snd_page_alloc 
mxser_new rtc ext3 jbd mbcache sd_mod ide_cd cdrom usb_storage usbhid ata_piix 
libata piix scsi_mod generic uhci_hcd ide_core usbcore skge thermal processor 
fan
CPU:0
EIP:0060:[]Tainted: P  VLI
EFLAGS: 00010246   (2.6.18-3-686 #1) 
EIP is at mxser_stoprx+0x54/0x74 [mxser_new]
eax:    ebx: 0001   ecx: f8f3d79c   edx: dfde7240
esi: f391211b   edi: f4f83fff   ebp: c1b17800   esp: f5bc7d7c
ds: 007b   es: 007b   ss: 0068
Process make (pid: 17088, ti=f5bc6000 task=f4febaa0 task.ti=f5bc6000)
Stack: 0001 c01fe4af 00ff f391201c c1b17c04 c011669e   
   0003 0096 c1b1780c 0286 0001 0096 c01f97c9  
   f456f800 0286 f8f38335 f5bc7e14 f8f3d8a8 0286 c01f9806 f456f800 
Call Trace:
 [] n_tty_receive_buf+0xcd6/0xcf9
 [] __wake_up+0x2a/0x3d
 [] tty_ldisc_deref+0x50/0x5f
 [] mxser_receive_chars+0x241/0x249 [mxser_new]
 [] tty_ldisc_try+0x2e/0x32
 [] flush_to_ldisc+0x12f/0x15c
 [] tty_ldisc_deref+0x1a/0x5f
 [] mxser_receive_chars+0x241/0x249 [mxser_new]
 [] mxser_transmit_chars+0x14/0x164 [mxser_new]
 [] flush_to_ldisc+0x104/0x15c
 [] mxser_receive_chars+0x241/0x249 [mxser_new]
 [] mxser_interrupt+0x15f/0x1e6 [mxser_new]
 [] handle_IRQ_event+0x23/0x49
 [] __do_IRQ+0x93/0xe8
 [] do_IRQ+0x43/0x52
 [] common_interrupt+0x1a/0x20
 [] __copy_from_user_ll+0x19/0x38
 [] convert_fxsr_from_user+0x15/0xd5
 [] pipe_read+0x0/0x1e
 [] restore_i387+0x6f/0xc0
 [] restore_sigcontext+0x10f/0x165
 [] sys_sigreturn+0xa4/0xcb
 [] syscall_call+0x7/0xb
Code: 83 e0 ee 89 41 3c 25 ee 00 00 00 42 eb 19 0f b6 42 1a 8b 51 08 89 41 38 
31 c0 42 ee 89 d8 83 c8 02 89 41 3c 0f b6 c0 ee 8b 41 04 <8b> 40 68 83 78 08 00 
79 15 8b 41 40 8b 51 08 83 e0 fd 89 41 40 
EIP: [] mxser_stoprx+0x54/0x74 [mxser_new] SS:ESP 0068:f5bc7d7c
 <0>Kernel panic - not syncing: Fatal exception in interrupt
 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: moxa serial driver testing

[Sergei Organov]

Jiri Slaby [EMAIL PROTECTED] writes:
 [EMAIL PROTECTED] wrote:
 Hi Jiri,
 
 I've figured out that both old and new mxser drivers have two similar
 problems:
 
 1. When there are data coming to a port, sometimes opening of the port
entirely locks the box. This is quite reproducible. Any idea what's
wrong and how can I help to debug it?

 Could you test the patch below, if something changes?

I've tested the latest version you've sent me yesterday. Still no
luck. The oops is below. I'll try with low_latency commented in a few
minutes.

BUG: unable to handle kernel NULL pointer dereference at virtual address 
0068
 printing eip:
f8f3711f
*pde = 
Oops:  [#1]
SMP 
Modules linked in: nvidia agpgart ipv6 nfs lockd nfs_acl sunrpc dm_mod sr_mod 
sbp2 ieee1394 ide_generic ide_disk e1000 snd_hda_intel snd_hda_codec 
snd_pcm_oss snd_mixer_oss i2c_i801 psmouse snd_pcm floppy tsdev serio_raw 
snd_timer parport_pc parport snd i2c_core pcspkr evdev soundcore snd_page_alloc 
mxser_new rtc ext3 jbd mbcache sd_mod ide_cd cdrom usb_storage usbhid ata_piix 
libata piix scsi_mod generic uhci_hcd ide_core usbcore skge thermal processor 
fan
CPU:0
EIP:0060:[f8f3711f]Tainted: P  VLI
EFLAGS: 00010246   (2.6.18-3-686 #1) 
EIP is at mxser_stoprx+0x54/0x74 [mxser_new]
eax:    ebx: 0001   ecx: f8f3d79c   edx: dfde7240
esi: f391211b   edi: f4f83fff   ebp: c1b17800   esp: f5bc7d7c
ds: 007b   es: 007b   ss: 0068
Process make (pid: 17088, ti=f5bc6000 task=f4febaa0 task.ti=f5bc6000)
Stack: 0001 c01fe4af 00ff f391201c c1b17c04 c011669e   
   0003 0096 c1b1780c 0286 0001 0096 c01f97c9  
   f456f800 0286 f8f38335 f5bc7e14 f8f3d8a8 0286 c01f9806 f456f800 
Call Trace:
 [c01fe4af] n_tty_receive_buf+0xcd6/0xcf9
 [c011669e] __wake_up+0x2a/0x3d
 [c01f97c9] tty_ldisc_deref+0x50/0x5f
 [f8f38335] mxser_receive_chars+0x241/0x249 [mxser_new]
 [c01f9806] tty_ldisc_try+0x2e/0x32
 [c01f9d4c] flush_to_ldisc+0x12f/0x15c
 [c01f9793] tty_ldisc_deref+0x1a/0x5f
 [f8f38335] mxser_receive_chars+0x241/0x249 [mxser_new]
 [f8f37678] mxser_transmit_chars+0x14/0x164 [mxser_new]
 [c01f9d21] flush_to_ldisc+0x104/0x15c
 [f8f38335] mxser_receive_chars+0x241/0x249 [mxser_new]
 [f8f38d7a] mxser_interrupt+0x15f/0x1e6 [mxser_new]
 [c013fb83] handle_IRQ_event+0x23/0x49
 [c013fc3c] __do_IRQ+0x93/0xe8
 [c01050e5] do_IRQ+0x43/0x52
 [c01036b6] common_interrupt+0x1a/0x20
 [c01bacf4] __copy_from_user_ll+0x19/0x38
 [c01071de] convert_fxsr_from_user+0x15/0xd5
 [c0164bc8] pipe_read+0x0/0x1e
 [c010774e] restore_i387+0x6f/0xc0
 [c0102203] restore_sigcontext+0x10f/0x165
 [c0102b3d] sys_sigreturn+0xa4/0xcb
 [c0102c7b] syscall_call+0x7/0xb
Code: 83 e0 ee 89 41 3c 25 ee 00 00 00 42 eb 19 0f b6 42 1a 8b 51 08 89 41 38 
31 c0 42 ee 89 d8 83 c8 02 89 41 3c 0f b6 c0 ee 8b 41 04 8b 40 68 83 78 08 00 
79 15 8b 41 40 8b 51 08 83 e0 fd 89 41 40 
EIP: [f8f3711f] mxser_stoprx+0x54/0x74 [mxser_new] SS:ESP 0068:f5bc7d7c
 0Kernel panic - not syncing: Fatal exception in interrupt
 
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: moxa serial driver testing

[Sergei Organov]

Jiri Slaby [EMAIL PROTECTED] writes:

 [EMAIL PROTECTED] wrote:
 Hi Jiri,
 
 I've figured out that both old and new mxser drivers have two similar
 problems:
 
 1. When there are data coming to a port, sometimes opening of the port
entirely locks the box. This is quite reproducible. Any idea what's
wrong and how can I help to debug it?

 Could you test the patch below, if something changes?

Just tested with low_latency commented out. Still oopses:

BUG: unable to handle kernel NULL pointer dereference at virtual address 
0008
 printing eip:
f8f1730f
*pde = 
Oops:  [#1]
SMP 
Modules linked in: nvidia agpgart ipv6 nfs lockd nfs_acl sunrpc dm_mod sr_mod 
sbp2 ieee1394 ide_generic ide_disk e1000 snd_hda_intel snd_hda_codec 
snd_pcm_oss snd_mixer_oss snd_pcm snd_timer i2c_i801 tsdev psmouse snd 
soundcore snd_page_alloc i2c_core serio_raw parport_pc parport mxser_new evdev 
floppy pcspkr rtc ext3 jbd mbcache usb_storage usbhid ide_cd cdrom sd_mod 
uhci_hcd piix usbcore skge ata_piix libata scsi_mod generic ide_core thermal 
processor fan
CPU:0
EIP:0060:[f8f1730f]Tainted: P  VLI
EFLAGS: 00010046   (2.6.18-3-686 #1) 
EIP is at mxser_receive_chars+0x21b/0x249 [mxser_new]
eax:    ebx:    ecx:    edx: 0286
esi: f8f1c79c   edi: 0001   ebp: c1be6000   esp: c0313efc
ds: 007b   es: 007b   ss: 0068
Process swapper (pid: 0, ti=c0312000 task=c02c76a0 task.ti=c0312000)
Stack: c0313f48 f8f1c8a8 903d  0fff 00ff 0286 007b 
   f8f1c79c 0006 007f 00c6 f8f17d7a 0007 0008 0080 
     f8f1bdc0 0060 df985500   003a 
Call Trace:
 [f8f17d7a] mxser_interrupt+0x15f/0x1e6 [mxser_new]
 [c013fb83] handle_IRQ_event+0x23/0x49
 [c013fc3c] __do_IRQ+0x93/0xe8
 [c01050e5] do_IRQ+0x43/0x52
 [c01036b6] common_interrupt+0x1a/0x20
 [c0101b91] mwait_idle+0x25/0x38
 [c0101b52] cpu_idle+0x9f/0xb9
 [c03186fd] start_kernel+0x379/0x380
Code: ed ff ff eb 1f 8b 06 83 78 18 00 75 17 8b 56 08 83 c2 05 ec 8b 14 24 0f 
b6 c0 a8 01 89 02 0f 85 d4 fe ff ff 8b 46 04 8b 54 24 18 8b 40 08 01 3c 85 c4 
ec f1 f8 8b 44 24 04 01 be f4 00 00 00 01 
EIP: [f8f1730f] mxser_receive_chars+0x21b/0x249 [mxser_new] SS:ESP 
0068:c0313efc
 0Kernel panic - not syncing: Fatal exception in interrupt
 BUG: warning at arch/i386/kernel/smp.c:547/smp_call_function()
 [c010f5a3] smp_call_function+0x53/0xfe
 [c011d97e] printk+0x14/0x18
 [c010f661] smp_send_stop+0x13/0x1c
 [c011cfc6] panic+0x4c/0xe2
 [c0104013] die+0x256/0x28a
 [c01156e0] do_page_fault+0x3b4/0x481
 [c01f9f8d] tty_buffer_request_room+0x107/0x112
 [c011532c] do_page_fault+0x0/0x481
 [c01037f9] error_code+0x39/0x40
 [f8f1730f] mxser_receive_chars+0x21b/0x249 [mxser_new]
 [f8f17d7a] mxser_interrupt+0x15f/0x1e6 [mxser_new]
 [c013fb83] handle_IRQ_event+0x23/0x49
 [c013fc3c] __do_IRQ+0x93/0xe8
 [c01050e5] do_IRQ+0x43/0x52
 [c01036b6] common_interrupt+0x1a/0x20
 [c0101b91] mwait_idle+0x25/0x38
 [c0101b52] cpu_idle+0x9f/0xb9
 [c03186fd] start_kernel+0x379/0x380
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: moxa serial driver testing

[Sergei Organov]

Jiri Slaby [EMAIL PROTECTED] writes:

 Jiri Slaby [EMAIL PROTECTED] writes:
 
  Could you test the patch below, if something changes?
 
 Just tested with low_latency commented out. Still oopses:
 
 BUG: unable to handle kernel NULL pointer dereference at virtual address 
 0008
  printing eip:
 f8f1730f
 *pde = 
 Oops:  [#1]
 SMP 
 Modules linked in: nvidia agpgart ipv6 nfs lockd nfs_acl sunrpc dm_mod 
 sr_mod sbp2 ieee1394 ide_generic ide_disk e1000 snd_hda_intel snd_hda_codec 
 snd_pcm_oss snd_mixer_oss snd_pcm snd_timer i2c_i801 tsdev psmouse snd 
 soundcore snd_page_alloc i2c_core serio_raw parport_pc parport mxser_new 
 evdev floppy pcspkr rtc ext3 jbd mbcache usb_storage usbhid ide_cd cdrom 
 sd_mod uhci_hcd piix usbcore skge ata_piix libata scsi_mod generic ide_core 
 thermal processor fan
 CPU:0
 EIP:0060:[f8f1730f]Tainted: P  VLI
 EFLAGS: 00010046   (2.6.18-3-686 #1) 
 EIP is at mxser_receive_chars+0x21b/0x249 [mxser_new]

 Yes, port-tty still somewhere becomes NULL -- does this patch help?

No, it still oopses (the oops is at the end).

In addition, before this latest patch was applied, when playing with two
simultaneously run programs that do nothing but open/close the port in a
loop (and the port was idle, i.e., nothing came to it from outside),
I've once got this thing:

irq 58: nobody cared (try booting with the irqpoll option)
 [c014037f] __report_bad_irq+0x2b/0x69
 [c014056c] note_interrupt+0x1af/0x1e7
 [c013fb83] handle_IRQ_event+0x23/0x49
 [c013fc5c] __do_IRQ+0xb3/0xe8
 [c01050e5] do_IRQ+0x43/0x52
 [c01036b6] common_interrupt+0x1a/0x20
 [c011007b] __cpu_up+0x88/0x164
 [c02810a0] _spin_unlock_irqrestore+0x8/0x9
 [f8f8bf57] mxser_startup+0x177/0x190 [mxser_new]
 [f8f8c3da] mxser_open+0x6b/0x251 [mxser_new]
 [c01fcafd] tty_open+0x16a/0x2e8
 [c01619a1] chrdev_open+0x126/0x141
 [c016187b] chrdev_open+0x0/0x141
 [c0158cb1] __dentry_open+0xc8/0x1ac
 [c0158df9] nameidata_to_filp+0x19/0x28
 [c0158e33] do_filp_open+0x2b/0x31
 [c0158e77] do_sys_open+0x3e/0xb3
 [c0158f19] sys_open+0x16/0x18
 [c0102c11] sysenter_past_esp+0x56/0x79
handlers:
[f8f8cc1b] (mxser_interrupt+0x0/0x1e6 [mxser_new])
Disabling IRQ #58

Another thing that I've noticed, is that when two programs that
open/close a port in a loop run simultaneously, each of them begin to
sometimes fail to open the port with errno=5 (I/O Errror). Though this
thing happends even if I try to use /dev/ttyS0, so it is not
mxser-specific (/dev/null never fails).

Here is the oops I promised at the beginning:

BUG: unable to handle kernel NULL pointer dereference at virtual address 
0008
 printing eip:
f927630f
*pde = 
Oops:  [#1]
SMP 
Modules linked in: mxser_new nvidia agpgart ipv6 nfs lockd nfs_acl sunrpc 
dm_mod sr_mod sbp2 ieee1394 ide_generic ide_disk e1000 snd_hda_intel 
snd_hda_codec snd_pcm_oss snd_mixer_oss snd_pcm psmouse snd_timer i2c_i801 snd 
serio_raw i2c_core tsdev parport_pc parport pcspkr evdev rtc soundcore 
snd_page_alloc floppy ext3 jbd mbcache sd_mod usb_storage usbhid ide_cd cdrom 
ata_piix libata scsi_mod uhci_hcd piix generic skge usbcore ide_core thermal 
processor fan
CPU:0
EIP:0060:[f927630f]Tainted: P  VLI
EFLAGS: 00010046   (2.6.18-3-686 #1) 
EIP is at mxser_receive_chars+0x21b/0x249 [mxser_new]
eax:    ebx:    ecx:    edx: 0286
esi: f927b79c   edi: 0001   ebp: c1991800   esp: c0313efc
ds: 007b   es: 007b   ss: 0068
Process swapper (pid: 0, ti=c0312000 task=c02c76a0 task.ti=c0312000)
Stack: c0313f48 f927b8a8 903a  0fff 00ff 0286 007b 
   f927b79c 0006 007f 00c6 f9276d7a 0007 0008 0080 
     f927adc0 0060 dfc74980   003a 
Call Trace:
 [f9276d7a] mxser_interrupt+0x15f/0x1e6 [mxser_new]
 [c013fb83] handle_IRQ_event+0x23/0x49
 [c013fc3c] __do_IRQ+0x93/0xe8
 [c01050e5] do_IRQ+0x43/0x52
 [c01036b6] common_interrupt+0x1a/0x20
 [c0101b91] mwait_idle+0x25/0x38
 [c0101b52] cpu_idle+0x9f/0xb9
 [c03186fd] start_kernel+0x379/0x380
Code: ed ff ff eb 1f 8b 06 83 78 18 00 75 17 8b 56 08 83 c2 05 ec 8b 14 24 0f 
b6 c0 a8 01 89 02 0f 85 d4 fe ff ff 8b 46 04 8b 54 24 18 8b 40 08 01 3c 85 c4 
dc 27 f9 8b 44 24 04 01 be f4 00 00 00 01 
EIP: [f927630f] mxser_receive_chars+0x21b/0x249 [mxser_new] SS:ESP 
0068:c0313efc
 0Kernel panic - not syncing: Fatal exception in interrupt
 BUG: warning at arch/i386/kernel/smp.c:547/smp_call_function()
 [c010f5a3] smp_call_function+0x53/0xfe
 [c011d97e] printk+0x14/0x18
 [c010f661] smp_send_stop+0x13/0x1c
 [c011cfc6] panic+0x4c/0xe2
 [c0104013] die+0x256/0x28a
 [c01156e0] do_page_fault+0x3b4/0x481
 [c01f9f8d] tty_buffer_request_room+0x107/0x112
 [c011532c] do_page_fault+0x0/0x481
 [c01037f9] error_code+0x39/0x40
 [f927630f] mxser_receive_chars+0x21b/0x249 [mxser_new]
 [f9276d7a] mxser_interrupt+0x15f/0x1e6 [mxser_new]
 [c013fb83] handle_IRQ_event+0x23/0x49
 [c013fc3c] __do_IRQ+0x93/0xe8
 [c01050e5] do_IRQ+0x43/0x52
 

Re: moxa serial driver testing

[Sergei Organov]

Jiri Slaby [EMAIL PROTECTED] writes:

 Jiri Slaby [EMAIL PROTECTED] writes:
 
  Could you test the patch below, if something changes?
 
 Just tested with low_latency commented out. Still oopses:
 
 BUG: unable to handle kernel NULL pointer dereference at virtual address 
 0008
  printing eip:
 f8f1730f
 *pde = 
 Oops:  [#1]
 SMP 
 Modules linked in: nvidia agpgart ipv6 nfs lockd nfs_acl sunrpc dm_mod 
 sr_mod sbp2 ieee1394 ide_generic ide_disk e1000 snd_hda_intel snd_hda_codec 
 snd_pcm_oss snd_mixer_oss snd_pcm snd_timer i2c_i801 tsdev psmouse snd 
 soundcore snd_page_alloc i2c_core serio_raw parport_pc parport mxser_new 
 evdev floppy pcspkr rtc ext3 jbd mbcache usb_storage usbhid ide_cd cdrom 
 sd_mod uhci_hcd piix usbcore skge ata_piix libata scsi_mod generic ide_core 
 thermal processor fan
 CPU:0
 EIP:0060:[f8f1730f]Tainted: P  VLI
 EFLAGS: 00010046   (2.6.18-3-686 #1) 
 EIP is at mxser_receive_chars+0x21b/0x249 [mxser_new]

 Yes, port-tty still somewhere becomes NULL -- does this patch help?

In addition to my previous answer. I've performed the same tests with
regular PC serial port. The issues with Disabling Irq #N and with I/O
error on open() exist with this port as well, so they aren't
mxser-specific.  I wasn't able to reproduce oopses, so they do seem to
be mxser-specific.

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: moxa serial driver testing

[Sergei Organov]

Jiri Slaby <[EMAIL PROTECTED]> writes:

> Sergei Organov wrote:
>> Jiri Slaby <[EMAIL PROTECTED]> writes:
>> 
>>> [EMAIL PROTECTED] wrote:
>>>> Hi Jiri,
>>>>
>>>> I've figured out that both old and new mxser drivers have two similar
>>>> problems:
>>>>
>>>> 1. When there are data coming to a port, sometimes opening of the port
>>>>entirely locks the box. This is quite reproducible. Any idea what's
>>>>wrong and how can I help to debug it?
>>> Could you test the patch below, if something changes?
>> 
>> Something did change. Now it becomes rather difficult to get the box to
>> hang, though not impossible. Another thing that changed is that now I
>> can see [parts of] oopses on screen. I've got two pictures of the screen
>
> Positive.
>
>> with different oopses. If you need them, let me know and I'll send them
>> to you in a separate mail not to pollute lkml with JPEGs.
>
> These are those you've posted in another post?

Not exactly. The oopses seem to vary from run to run, so I can't get
exactly the same every time. Though I kept those JPEGs, if you are still
interested after looking at those oopses that I've collected through
serial console and sent.

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: moxa serial driver testing (oopses)

[Sergei Organov]

Jiri Slaby <[EMAIL PROTECTED]> writes:
> [EMAIL PROTECTED] wrote:
>> Hi Jiri,
>> 
>> I've figured out that both old and new mxser drivers have two similar
>> problems:
>> 
>> 1. When there are data coming to a port, sometimes opening of the port
>>entirely locks the box. This is quite reproducible. Any idea what's
>>wrong and how can I help to debug it?
>
> Could you test the patch below, if something changes?

In addition to my previous answer, fortunately I was able to log oopses
to a serial console, so below are two of them.

They seem to appear from in a few seconds to in a few minutes after I
run:

$ while true; do cat /dev/ttyM7 > /dev/null; done

when /dev/ttyM7 is setup so that 'cat' immediately returns (due to zero
timeout).

Note that unpatched version always hangs in less than a second.

First oops is taken from the kernel with lock debugging enabled:

BUG: unable to handle kernel NULL pointer dereference at virtual address 
0080
 printing eip:
f8fa5136
*pde = 
Oops:  [#1]
SMP 
Modules linked in: ipv6 nfs lockd nfs_acl sunrpc dm_mod sr_mod sbp2 ieee1394 
ide_generic ide_disk e1000 snd_hda_intel snd_hda_codec snd_pcm_oss 
snd_mixer_oss snd_pcm snd_timer snd evdev mxser_new tsdev soundcore psmouse 
snd_page_alloc serio_raw floppy i2c_i801 parport_pc parport pcspkr i2c_core rtc 
ext3 jbd mbcache usb_storage sd_mod usbhid ide_cd cdrom ata_piix libata 
uhci_hcd scsi_mod piix generic usbcore ide_core skge thermal processor fan
CPU:0
EIP:0060:[]Not tainted VLI
EFLAGS: 00010046   (2.6.18 #1) 
EIP is at mxser_stoprx+0x5a/0x7e [mxser_new]
eax:    ebx: f7a55000   ecx: f8fabc1c   edx: f76f6340
esi: c1e4651b   edi: eb466fff   ebp: f3af3de8   esp: f3af3de4
ds: 007b   es: 007b   ss: 0068
Process cc1plus (pid: 9540, ti=f3af2000 task=f4012ab0 task.ti=f3af2000)
Stack: f7a55000 f3af3df0 f8fa5162 f3af3ec0 c020aedb 00ff c1e4641c f7a554dc 
   0002 0008  0008 f4012ff0 0001 017d  
   017d f4012ff0 0001 017d  f4012ff0 017d  
Call Trace:
 [] show_stack_log_lvl+0x8c/0x97
 [] show_registers+0x130/0x19d
 [] die+0x181/0x287
 [] do_page_fault+0x3ca/0x49e
 [] error_code+0x39/0x40
 [] mxser_throttle+0x8/0xa [mxser_new]
 [] n_tty_receive_buf+0xdce/0xdf0
 [] flush_to_ldisc+0x112/0x149
 [] tty_flip_buffer_push+0x3d/0x53
 [] mxser_receive_chars+0x23c/0x244 [mxser_new]
 [] mxser_interrupt+0x14c/0x1cb [mxser_new]
 [] handle_IRQ_event+0x20/0x4d
 [] __do_IRQ+0x94/0xef
 [] do_IRQ+0x4e/0x60
 [] common_interrupt+0x25/0x2c
Code: 83 e0 ee 89 41 3c 25 ee 00 00 00 42 eb 19 0f b6 42 1a 8b 51 08 89 41 38 
31 c0 42 ee 89 d8 83 c8 02 89 41 3c 0f b6 c0 ee 8b 41 04 <8b> 80 80 00 00 00 83 
78 08 00 79 15 8b 41 40 8b 51 08 83 e0 fd 
EIP: [] mxser_stoprx+0x5a/0x7e [mxser_new] SS:ESP 0068:f3af3de4
 <0>Kernel panic - not syncing: Fatal exception in interrupt
 BUG: warning at arch/i386/kernel/smp.c:547/smp_call_function()
 [] show_trace+0xd/0x10
 [] dump_stack+0x19/0x1b
 [] smp_call_function+0x55/0xfd
 [] smp_send_stop+0x16/0x2a
 [] panic+0x4d/0xec
 [] die+0x252/0x287
 [] do_page_fault+0x3ca/0x49e
 [] error_code+0x39/0x40
 [] mxser_throttle+0x8/0xa [mxser_new]
 [] n_tty_receive_buf+0xdce/0xdf0
 [] flush_to_ldisc+0x112/0x149
 [] tty_flip_buffer_push+0x3d/0x53
 [] mxser_receive_chars+0x23c/0x244 [mxser_new]
 [] mxser_interrupt+0x14c/0x1cb [mxser_new]
 [] handle_IRQ_event+0x20/0x4d
 [] __do_IRQ+0x94/0xef
 [] do_IRQ+0x4e/0x60
 [] common_interrupt+0x25/0x2c

Another oops is form kernel with lock debugging disabled:

BUG: unable to handle kernel NULL pointer dereference at virtual address 
0068
 printing eip:
f8f0911f
*pde = 
Oops:  [#1]
SMP 
Modules linked in: nvidia agpgart ipv6 nfs lockd nfs_acl sunrpc dm_mod sr_mod 
sbp2 ieee1394 ide_generic ide_disk e1000 snd_hda_intel snd_hda_codec 
snd_pcm_oss snd_mixer_oss snd_pcm i2c_i801 psmouse snd_timer rtc parport_pc 
serio_raw i2c_core snd soundcore snd_page_alloc evdev pcspkr floppy parport 
tsdev mxser_new ext3 jbd mbcache usb_storage usbhid sd_mod ide_cd cdrom 
uhci_hcd ata_piix usbcore piix skge libata scsi_mod generic ide_core thermal 
processor fan
CPU:0
EIP:0060:[]Tainted: P  VLI
EFLAGS: 00010246   (2.6.18-3-686 #1) 
EIP is at mxser_stoprx+0x54/0x74 [mxser_new]
eax:    ebx: 0001   ecx: f8f0f79c   edx: f643a4c0
esi: efe6d51b   edi: ee75dfff   ebp: f6e2e000   esp: ec901e20
ds: 007b   es: 007b   ss: 0068
Process cc1plus (pid: 17764, ti=ec90 task=dff41000 task.ti=ec90)
Stack: 0001 c01fe4af 00ff efe6d41c f6e2e404 c02d6c68  0001 
   ec901e64 c011669e   0003 0086 f6e2e00c 0286 
   0001 0086 c01f97c9  f524bc00 0286 f8f0a335 ec901ec8 
Call Trace:
 [] n_tty_receive_buf+0xcd6/0xcf9
 [] __wake_up+0x2a/0x3d
 [] tty_ldisc_deref+0x50/0x5f
 [] mxser_receive_chars+0x241/0x249 [mxser_new]
 [] mxser_transmit_chars+0x14/0x164 [mxser_new]
 [] mxser_interrupt+0x190/0x1e6 

Re: moxa serial driver testing

[Sergei Organov]

Jiri Slaby <[EMAIL PROTECTED]> writes:

> [EMAIL PROTECTED] wrote:
>> Hi Jiri,
>> 
>> I've figured out that both old and new mxser drivers have two similar
>> problems:
>> 
>> 1. When there are data coming to a port, sometimes opening of the port
>>entirely locks the box. This is quite reproducible. Any idea what's
>>wrong and how can I help to debug it?
>
> Could you test the patch below, if something changes?

Something did change. Now it becomes rather difficult to get the box to
hang, though not impossible. Another thing that changed is that now I
can see [parts of] oopses on screen. I've got two pictures of the screen
with different oopses. If you need them, let me know and I'll send them
to you in a separate mail not to pollute lkml with JPEGs.

One of these oopses happened on kernel with lock debugging enabled, but
I can't see anything relevant in dmesg log after resetting the box.

SysRq magic still doesn't work after hang.

As for the problem with module unloading when port is open, your another
patch does fix it indeed.

-- Sergei.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: moxa serial driver testing

[Sergei Organov]

Jiri Slaby <[EMAIL PROTECTED]> writes:

> [EMAIL PROTECTED] wrote:
>> Hi Jiri,
>> 
>> I've figured out that both old and new mxser drivers have two similar
>> problems:
>> 
>> 1. When there are data coming to a port, sometimes opening of the port
>>entirely locks the box. This is quite reproducible. Any idea what's
>>wrong and how can I help to debug it?
>
> Could you test the patch below, if something changes?

I'm preparing to test it. However, it seems that my version of
mxser_new.c got out of sync with your one:

[EMAIL PROTECTED] mxser_new$ patch -p3 < lock.patch
patching file mxser_new.c
Hunk #1 succeeded at 1900 (offset -368 lines).
Hunk #2 succeeded at 1968 (offset -354 lines).
[EMAIL PROTECTED] mxser_new$ patch -p3 < rmmod.patch
patching file mxser.c
Hunk #1 succeeded at 711 with fuzz 2 (offset -6 lines).
patching file mxser_new.c
Hunk #1 succeeded at 705 with fuzz 2 (offset -1985 lines).
[EMAIL PROTECTED] mxser_new$

I'll try this one anyway, but could you please either tell me where to
get the version you are using, or just send it to me by mail?

BTW, when system hangs, SysRq magic doesn't work anymore.

-- Sergei.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: moxa serial driver testing

[Sergei Organov]

Jiri Slaby [EMAIL PROTECTED] writes:

 [EMAIL PROTECTED] wrote:
 Hi Jiri,
 
 I've figured out that both old and new mxser drivers have two similar
 problems:
 
 1. When there are data coming to a port, sometimes opening of the port
entirely locks the box. This is quite reproducible. Any idea what's
wrong and how can I help to debug it?

 Could you test the patch below, if something changes?

I'm preparing to test it. However, it seems that my version of
mxser_new.c got out of sync with your one:

[EMAIL PROTECTED] mxser_new$ patch -p3  lock.patch
patching file mxser_new.c
Hunk #1 succeeded at 1900 (offset -368 lines).
Hunk #2 succeeded at 1968 (offset -354 lines).
[EMAIL PROTECTED] mxser_new$ patch -p3  rmmod.patch
patching file mxser.c
Hunk #1 succeeded at 711 with fuzz 2 (offset -6 lines).
patching file mxser_new.c
Hunk #1 succeeded at 705 with fuzz 2 (offset -1985 lines).
[EMAIL PROTECTED] mxser_new$

I'll try this one anyway, but could you please either tell me where to
get the version you are using, or just send it to me by mail?

BTW, when system hangs, SysRq magic doesn't work anymore.

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: moxa serial driver testing

[Sergei Organov]

Jiri Slaby [EMAIL PROTECTED] writes:

 [EMAIL PROTECTED] wrote:
 Hi Jiri,
 
 I've figured out that both old and new mxser drivers have two similar
 problems:
 
 1. When there are data coming to a port, sometimes opening of the port
entirely locks the box. This is quite reproducible. Any idea what's
wrong and how can I help to debug it?

 Could you test the patch below, if something changes?

Something did change. Now it becomes rather difficult to get the box to
hang, though not impossible. Another thing that changed is that now I
can see [parts of] oopses on screen. I've got two pictures of the screen
with different oopses. If you need them, let me know and I'll send them
to you in a separate mail not to pollute lkml with JPEGs.

One of these oopses happened on kernel with lock debugging enabled, but
I can't see anything relevant in dmesg log after resetting the box.

SysRq magic still doesn't work after hang.

As for the problem with module unloading when port is open, your another
patch does fix it indeed.

-- Sergei.

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: moxa serial driver testing (oopses)

[Sergei Organov]

Jiri Slaby [EMAIL PROTECTED] writes:
 [EMAIL PROTECTED] wrote:
 Hi Jiri,
 
 I've figured out that both old and new mxser drivers have two similar
 problems:
 
 1. When there are data coming to a port, sometimes opening of the port
entirely locks the box. This is quite reproducible. Any idea what's
wrong and how can I help to debug it?

 Could you test the patch below, if something changes?

In addition to my previous answer, fortunately I was able to log oopses
to a serial console, so below are two of them.

They seem to appear from in a few seconds to in a few minutes after I
run:

$ while true; do cat /dev/ttyM7  /dev/null; done

when /dev/ttyM7 is setup so that 'cat' immediately returns (due to zero
timeout).

Note that unpatched version always hangs in less than a second.

First oops is taken from the kernel with lock debugging enabled:

BUG: unable to handle kernel NULL pointer dereference at virtual address 
0080
 printing eip:
f8fa5136
*pde = 
Oops:  [#1]
SMP 
Modules linked in: ipv6 nfs lockd nfs_acl sunrpc dm_mod sr_mod sbp2 ieee1394 
ide_generic ide_disk e1000 snd_hda_intel snd_hda_codec snd_pcm_oss 
snd_mixer_oss snd_pcm snd_timer snd evdev mxser_new tsdev soundcore psmouse 
snd_page_alloc serio_raw floppy i2c_i801 parport_pc parport pcspkr i2c_core rtc 
ext3 jbd mbcache usb_storage sd_mod usbhid ide_cd cdrom ata_piix libata 
uhci_hcd scsi_mod piix generic usbcore ide_core skge thermal processor fan
CPU:0
EIP:0060:[f8fa5136]Not tainted VLI
EFLAGS: 00010046   (2.6.18 #1) 
EIP is at mxser_stoprx+0x5a/0x7e [mxser_new]
eax:    ebx: f7a55000   ecx: f8fabc1c   edx: f76f6340
esi: c1e4651b   edi: eb466fff   ebp: f3af3de8   esp: f3af3de4
ds: 007b   es: 007b   ss: 0068
Process cc1plus (pid: 9540, ti=f3af2000 task=f4012ab0 task.ti=f3af2000)
Stack: f7a55000 f3af3df0 f8fa5162 f3af3ec0 c020aedb 00ff c1e4641c f7a554dc 
   0002 0008  0008 f4012ff0 0001 017d  
   017d f4012ff0 0001 017d  f4012ff0 017d  
Call Trace:
 [c0103e2f] show_stack_log_lvl+0x8c/0x97
 [c0103fa6] show_registers+0x130/0x19d
 [c0104194] die+0x181/0x287
 [c0115b92] do_page_fault+0x3ca/0x49e
 [c01039cd] error_code+0x39/0x40
 [f8fa5162] mxser_throttle+0x8/0xa [mxser_new]
 [c020aedb] n_tty_receive_buf+0xdce/0xdf0
 [c02062fc] flush_to_ldisc+0x112/0x149
 [c0206370] tty_flip_buffer_push+0x3d/0x53
 [f8fa63c4] mxser_receive_chars+0x23c/0x244 [mxser_new]
 [f8fa6e48] mxser_interrupt+0x14c/0x1cb [mxser_new]
 [c014563a] handle_IRQ_event+0x20/0x4d
 [c01456fb] __do_IRQ+0x94/0xef
 [c0105342] do_IRQ+0x4e/0x60
 [c0103835] common_interrupt+0x25/0x2c
Code: 83 e0 ee 89 41 3c 25 ee 00 00 00 42 eb 19 0f b6 42 1a 8b 51 08 89 41 38 
31 c0 42 ee 89 d8 83 c8 02 89 41 3c 0f b6 c0 ee 8b 41 04 8b 80 80 00 00 00 83 
78 08 00 79 15 8b 41 40 8b 51 08 83 e0 fd 
EIP: [f8fa5136] mxser_stoprx+0x5a/0x7e [mxser_new] SS:ESP 0068:f3af3de4
 0Kernel panic - not syncing: Fatal exception in interrupt
 BUG: warning at arch/i386/kernel/smp.c:547/smp_call_function()
 [c0103e73] show_trace+0xd/0x10
 [c010444c] dump_stack+0x19/0x1b
 [c010f600] smp_call_function+0x55/0xfd
 [c010f6be] smp_send_stop+0x16/0x2a
 [c011d885] panic+0x4d/0xec
 [c0104265] die+0x252/0x287
 [c0115b92] do_page_fault+0x3ca/0x49e
 [c01039cd] error_code+0x39/0x40
 [f8fa5162] mxser_throttle+0x8/0xa [mxser_new]
 [c020aedb] n_tty_receive_buf+0xdce/0xdf0
 [c02062fc] flush_to_ldisc+0x112/0x149
 [c0206370] tty_flip_buffer_push+0x3d/0x53
 [f8fa63c4] mxser_receive_chars+0x23c/0x244 [mxser_new]
 [f8fa6e48] mxser_interrupt+0x14c/0x1cb [mxser_new]
 [c014563a] handle_IRQ_event+0x20/0x4d
 [c01456fb] __do_IRQ+0x94/0xef
 [c0105342] do_IRQ+0x4e/0x60
 [c0103835] common_interrupt+0x25/0x2c

Another oops is form kernel with lock debugging disabled:

BUG: unable to handle kernel NULL pointer dereference at virtual address 
0068
 printing eip:
f8f0911f
*pde = 
Oops:  [#1]
SMP 
Modules linked in: nvidia agpgart ipv6 nfs lockd nfs_acl sunrpc dm_mod sr_mod 
sbp2 ieee1394 ide_generic ide_disk e1000 snd_hda_intel snd_hda_codec 
snd_pcm_oss snd_mixer_oss snd_pcm i2c_i801 psmouse snd_timer rtc parport_pc 
serio_raw i2c_core snd soundcore snd_page_alloc evdev pcspkr floppy parport 
tsdev mxser_new ext3 jbd mbcache usb_storage usbhid sd_mod ide_cd cdrom 
uhci_hcd ata_piix usbcore piix skge libata scsi_mod generic ide_core thermal 
processor fan
CPU:0
EIP:0060:[f8f0911f]Tainted: P  VLI
EFLAGS: 00010246   (2.6.18-3-686 #1) 
EIP is at mxser_stoprx+0x54/0x74 [mxser_new]
eax:    ebx: 0001   ecx: f8f0f79c   edx: f643a4c0
esi: efe6d51b   edi: ee75dfff   ebp: f6e2e000   esp: ec901e20
ds: 007b   es: 007b   ss: 0068
Process cc1plus (pid: 17764, ti=ec90 task=dff41000 task.ti=ec90)
Stack: 0001 c01fe4af 00ff efe6d41c f6e2e404 c02d6c68  0001 
   ec901e64 c011669e   0003 0086 f6e2e00c 0286 
   0001 0086 c01f97c9  f524bc00 

Re: moxa serial driver testing

[Sergei Organov]

Jiri Slaby [EMAIL PROTECTED] writes:

 Sergei Organov wrote:
 Jiri Slaby [EMAIL PROTECTED] writes:
 
 [EMAIL PROTECTED] wrote:
 Hi Jiri,

 I've figured out that both old and new mxser drivers have two similar
 problems:

 1. When there are data coming to a port, sometimes opening of the port
entirely locks the box. This is quite reproducible. Any idea what's
wrong and how can I help to debug it?
 Could you test the patch below, if something changes?
 
 Something did change. Now it becomes rather difficult to get the box to
 hang, though not impossible. Another thing that changed is that now I
 can see [parts of] oopses on screen. I've got two pictures of the screen

 Positive.

 with different oopses. If you need them, let me know and I'll send them
 to you in a separate mail not to pollute lkml with JPEGs.

 These are those you've posted in another post?

Not exactly. The oopses seem to vary from run to run, so I can't get
exactly the same every time. Though I kept those JPEGs, if you are still
interested after looking at those oopses that I've collected through
serial console and sent.

-- Sergei.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: moxa serial driver testing

[Sergei Organov]

Jiri,

Jiri Slaby <[EMAIL PROTECTED]> writes:
> [EMAIL PROTECTED] wrote:
>> Hi Jiri,
>> 
>> I've figured out that both old and new mxser drivers have two similar
>> problems:
>> 
>> 1. When there are data coming to a port, sometimes opening of the port
>>entirely locks the box. This is quite reproducible. Any idea what's
>>wrong and how can I help to debug it?
>
> Could you test the patch below, if something changes?

Thanks for looking into it. I'll be able to get to the box with moxa
installed on Monday and will try the patch.

As for SysRq, I'm afraid it didn't work though I'm not 100% sure. I'll
check that as well.

-- Sergei.

> ---
>
>  drivers/char/mxser_new.c |6 --
>  1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/mxser_new.c b/drivers/char/mxser_new.c
> index a2bca5d..c0af201 100644
> --- a/drivers/char/mxser_new.c
> +++ b/drivers/char/mxser_new.c
> @@ -2268,6 +2268,8 @@ static irqreturn_t mxser_interrupt(int irq, void 
> *dev_id)
>   if (bits & irqbits)
>   continue;
>   port = >ports[i];
> + if (!(port->flags & ASYNC_INITIALIZED))
> + continue;
>  
>   int_cnt = 0;
>   do {
> @@ -2320,9 +2322,9 @@ static irqreturn_t mxser_interrupt(int irq, void 
> *dev_id)
>   if (status & UART_LSR_THRE)
>   mxser_transmit_chars(port);
>   }
> - } while (int_cnt++ < MXSER_ISR_PASS_LIMIT);
> + } while (int_cnt++ < 256);
>   }
> - if (pass_counter++ > MXSER_ISR_PASS_LIMIT)
> + if (pass_counter++ > 64)
>   break;  /* Prevent infinite loops */
>   }
>  
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: moxa serial driver testing

[Sergei Organov]

Jiri,

Jiri Slaby [EMAIL PROTECTED] writes:
 [EMAIL PROTECTED] wrote:
 Hi Jiri,
 
 I've figured out that both old and new mxser drivers have two similar
 problems:
 
 1. When there are data coming to a port, sometimes opening of the port
entirely locks the box. This is quite reproducible. Any idea what's
wrong and how can I help to debug it?

 Could you test the patch below, if something changes?

Thanks for looking into it. I'll be able to get to the box with moxa
installed on Monday and will try the patch.

As for SysRq, I'm afraid it didn't work though I'm not 100% sure. I'll
check that as well.

-- Sergei.

 ---

  drivers/char/mxser_new.c |6 --
  1 files changed, 4 insertions(+), 2 deletions(-)

 diff --git a/drivers/char/mxser_new.c b/drivers/char/mxser_new.c
 index a2bca5d..c0af201 100644
 --- a/drivers/char/mxser_new.c
 +++ b/drivers/char/mxser_new.c
 @@ -2268,6 +2268,8 @@ static irqreturn_t mxser_interrupt(int irq, void 
 *dev_id)
   if (bits  irqbits)
   continue;
   port = brd-ports[i];
 + if (!(port-flags  ASYNC_INITIALIZED))
 + continue;
  
   int_cnt = 0;
   do {
 @@ -2320,9 +2322,9 @@ static irqreturn_t mxser_interrupt(int irq, void 
 *dev_id)
   if (status  UART_LSR_THRE)
   mxser_transmit_chars(port);
   }
 - } while (int_cnt++  MXSER_ISR_PASS_LIMIT);
 + } while (int_cnt++  256);
   }
 - if (pass_counter++  MXSER_ISR_PASS_LIMIT)
 + if (pass_counter++  64)
   break;  /* Prevent infinite loops */
   }
  
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Linux tty layer hackery: Heads up and RFC

[Sergei Organov]

Alan Cox <[EMAIL PROTECTED]> writes:
> At the moment tty buffers are attached directly to the tty. This is
> causing a lot of the problems related to tty layer locking, also
> problems at high speed and also with bursty data (such as occurs in
> virtualised environments)
> 
> I'm working on ripping out the flip buffers and replacing them with a
> pool of dynamically allocated buffers. This allows both for old style
> "byte I/O" devices and also helps virtualisation and smart devices where
> large blocks of data suddenely materialise and need storing.

Great! Really good news!

> 
> So far so good. Lots of drivers reference tty->flip.*. Several of them
> also call directly and unsafely into function pointers it provides. This
> will all break. Most drivers can use tty_insert_flip_char which can be
> kept as an API but others need more.
> 
> At the moment I've added the following interfaces, if people think more
> will be needed now is a good time to say
> 
> int tty_buffer_request_room(tty, size)
> 
> Try and ensure at least size bytes are available, returns actual room
> (may be zero). At the moment it just uses the flipbuf space but that
> will change. Repeated calls without characters being added are not
> cumulative. (ie if you call it with 1, 1, 1, and then 4 you'll have four
> characters of space. The other functions will also try and grow buffers
> in future but this will be a more efficient way when you know block
> sizes.
> 
> int tty_insert_flip_char(tty, ch, flag)
> 
> As before insert a character if there is room. Now returns 1 for
> success, 0 for failure.
> 
> int tty_insert_flip_string(tty, str, len)
> 
> Insert a block of non error characters. Returns the number inserted.
> 
> int tty_prepare_flip_string(tty, strptr, len)
> 
> Adjust the buffer to allow len characters to be added. Returns a buffer
> pointer in strptr and the length available. This allows for hardware
> that needs to use functions like insl or mencpy_fromio.

As you are going to replace flip buffers with different implementation
anyway, isn't it better to get rid of "flip" in the interface names as
well (maybe providing some synonyms for backward compatibility)? What I
mean is that names like

tty_buffer_insert_char()
tty_buffer_insert_string()

for the new interfaces would probably make more sense.

Otherwise I find the interfaces you suggest just fine and suitable for
the task I was unable to achieve without ugly hacks using current flip
buffers interfaces (reliable high-speed bulk USB tty driver).

-- 
Sergei.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Linux tty layer hackery: Heads up and RFC

[Sergei Organov]

Alan Cox [EMAIL PROTECTED] writes:
 At the moment tty buffers are attached directly to the tty. This is
 causing a lot of the problems related to tty layer locking, also
 problems at high speed and also with bursty data (such as occurs in
 virtualised environments)
 
 I'm working on ripping out the flip buffers and replacing them with a
 pool of dynamically allocated buffers. This allows both for old style
 byte I/O devices and also helps virtualisation and smart devices where
 large blocks of data suddenely materialise and need storing.

Great! Really good news!

 
 So far so good. Lots of drivers reference tty-flip.*. Several of them
 also call directly and unsafely into function pointers it provides. This
 will all break. Most drivers can use tty_insert_flip_char which can be
 kept as an API but others need more.
 
 At the moment I've added the following interfaces, if people think more
 will be needed now is a good time to say
 
 int tty_buffer_request_room(tty, size)
 
 Try and ensure at least size bytes are available, returns actual room
 (may be zero). At the moment it just uses the flipbuf space but that
 will change. Repeated calls without characters being added are not
 cumulative. (ie if you call it with 1, 1, 1, and then 4 you'll have four
 characters of space. The other functions will also try and grow buffers
 in future but this will be a more efficient way when you know block
 sizes.
 
 int tty_insert_flip_char(tty, ch, flag)
 
 As before insert a character if there is room. Now returns 1 for
 success, 0 for failure.
 
 int tty_insert_flip_string(tty, str, len)
 
 Insert a block of non error characters. Returns the number inserted.
 
 int tty_prepare_flip_string(tty, strptr, len)
 
 Adjust the buffer to allow len characters to be added. Returns a buffer
 pointer in strptr and the length available. This allows for hardware
 that needs to use functions like insl or mencpy_fromio.

As you are going to replace flip buffers with different implementation
anyway, isn't it better to get rid of flip in the interface names as
well (maybe providing some synonyms for backward compatibility)? What I
mean is that names like

tty_buffer_insert_char()
tty_buffer_insert_string()

for the new interfaces would probably make more sense.

Otherwise I find the interfaces you suggest just fine and suitable for
the task I was unable to achieve without ugly hacks using current flip
buffers interfaces (reliable high-speed bulk USB tty driver).

-- 
Sergei.

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Kernel SCM saga..

[Sergei Organov]

David Woodhouse <[EMAIL PROTECTED]> writes:

> On Wed, 2005-04-06 at 08:42 -0700, Linus Torvalds wrote:
> > PS. Don't bother telling me about subversion. If you must, start reading
> > up on "monotone". That seems to be the most viable alternative, but don't
> > pester the developers so much that they don't get any work done. They are
> > already aware of my problems ;)
> 
> One feature I'd want to see in a replacement version control system is
> the ability to _re-order_ patches, and to cherry-pick patches from my
> tree to be sent onwards. The lack of that capability is the main reason
> I always hated BitKeeper.

darcs? 

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/