netdev - Re: ixp4xx_hss modifications for 2x4 HDLC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Wed, 25 Feb 2009 09:43:17 +0100
From:	Miguel Ángel Álvarez <gotzoncabanes@...il.com>
To:	Krzysztof Halasa <khc@...waw.pl>
Cc:	linux-arm-kernel <linux-arm-kernel@...ts.arm.linux.org.uk>,
	netdev@...r.kernel.org
Subject: Re: ixp4xx_hss modifications for 2x4 HDLC

Hi

2009/2/25 Krzysztof Halasa <khc@...waw.pl>:
> Hi,
>
> Miguel Ángel Álvarez <gotzoncabanes@...il.com> writes:
>
>> I would appreciate your comments very much, because I think this
>> functionallity is quite usefull and I would like to add it to the main
>> kernel stream.
>
> Well, I guess it's a bit like a proof of concept rather than
> an applicable patch? :-)
>
Sure. I have made it work, and just want to explain how. And yes... I
would like to transform it into a valuable patch but need some
guidance.

> And it's based on "chan" branch. This isn't good, the "chan" isn't yet
> ready to go upstream, the interface isn't yet stable. It's really
> experimental, based on undocumented behaviour etc. Do you need the
> "channelized" feature? If not, perhaps you should stick to HDLC-only
> David's net-next?
>
I have not changed of test chan at all... But I didn't even know there
was another version. I have just began with the one in linux-2.6.26.

> Ok. The following doesn't IMHO make sense:
>
>> @@ -349,12 +294,58 @@
>>               .set_clock      = hss_set_clock,
>>               .open           = hss_open,
>>               .close          = hss_close,
>> -             .txreadyq       = 2,
>> +             .txreadyq       = 32,
>> +             .hss        = 0,
>> +             .hdlc       = 0,
>>       }, {
>>               .set_clock      = hss_set_clock,
>>               .open           = hss_open,
>>               .close          = hss_close,
>> -             .txreadyq       = 19,
>> +             .txreadyq       = 36,
>> +             .hss        = 1,
>> +             .hdlc       = 0,
>> +     }, {
>> +             .set_clock      = hss_set_clock,
>> +             .open           = hss_open,
>> +             .close          = hss_close,
>
> There are only two HSS devices on the chip. Not 8 :-)
>
Well... I am not sure about that. For me each hdlc is a different
network device, so I thing they should have their room in the platform
description... But maybe we can just have two hss, and describing in
each one the txreadyq of each hdlc. In any case... isn't this more
flexible as the user can decide which hdlcs to use?

>> @@ -367,6 +358,30 @@
>>               .name                   = "ixp4xx_hss",
>>               .id                     = 1,
>>               .dev.platform_data      = hss_plat + 1,
>> +     }, {
>> +             .name                   = "ixp4xx_hss",
>> +             .id                     = 2,
>> +             .dev.platform_data      = hss_plat + 2,
>> +     }, {
>> +             .name                   = "ixp4xx_hss",
>> +             .id                     = 3,
>> +             .dev.platform_data      = hss_plat + 3,
>
> Same here.
Same here

>
> This only needs a tiny change, increasing the number of HDLC devices
> to 2 or 4 (for each HSS), adding the extra queue numbers etc.

Yes... The changes are not much (I thought they were, but I have
refined the patch with time).
>
> This is completely orthogonal to:
> a) clocks
> b) channelized "services"
> c) sync options (excluding 1024-bit max length).

I agree, except that lut must also be taken into account.

>
> The clock generator is a different thing and needs to be investigated.
> Copying Intel's table doesn't make sense here (especially given the
> internal clocks are rarely used). Do you need internal clocks?
>

I would have liked to use internal clocks, but i obtained 7,8MHz for
the supposed 8.192 MHz configuration from Intel, so finally I had to
implement the external clock.

> Channelized thing needs a stable userspace interface first, it really
> have to be discussed on netdev.
>

I agree.

> Sync options are simply independent.
>

I agree.

> I know I promised you I'll look at this stuff soon. Unfortunately, that
> "soon" is a bit less "soon" than I though. Will look when able,
> currently I'm really overwhelmed.

I know... And I am not pretending you to do all the job. I suppose you
and me are not the only ones interested in HSS, so that is why I have
asked for the comments of anyone interested.

For me it was critical to make this work. Now it is working so I have
not as much pressure, but I do not want to forget this, and I want to
share my experience with others.

Miguel Ángel Álvarez
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html