linux-kernel - Re: [PATCH net-next 11/13] sunhme: Combine continued messages

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <cf05390e-a69b-ad34-8c61-c5e9bbdaf5e3@gmail.com>
Date:   Mon, 19 Sep 2022 10:14:34 -0400
From:   Sean Anderson <seanga2@...il.com>
To:     Rolf Eike Beer <eike-kernel@...tec.de>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org, Zheyu Ma <zheyuma97@...il.com>,
        Nick Bowler <nbowler@...conx.ca>
Subject: Re: [PATCH net-next 11/13] sunhme: Combine continued messages

On 9/19/22 09:23, Rolf Eike Beer wrote:
> Am Montag, 19. September 2022, 01:26:24 CEST schrieb Sean Anderson:
>> This driver seems to have been written under the assumption that messages
>> can be continued arbitrarily. I'm not when this changed (if ever), but such
>> ad-hoc continuations are liable to be rudely interrupted. Convert all such
>> instances to single prints. This loses a bit of timing information (such as
>> when a line was constructed piecemeal as the function executed), but it's
>> easy to add a few prints if necessary. This also adds newlines to the ends
>> of any prints without them.
> 
> I have a similar patch around, but yours catches more places.
> 
>> diff --git a/drivers/net/ethernet/sun/sunhme.c
>> b/drivers/net/ethernet/sun/sunhme.c index 98c38e213bab..9965c9c872a6 100644
>> --- a/drivers/net/ethernet/sun/sunhme.c
>> +++ b/drivers/net/ethernet/sun/sunhme.c
>> @@ -330,7 +331,6 @@ static int happy_meal_bb_read(struct happy_meal *hp,
>>   	int retval = 0;
>>   	int i;
>>
>> -	ASD("happy_meal_bb_read: reg=%d ", reg);
>>
>>   	/* Enable the MIF BitBang outputs. */
>>   	hme_write32(hp, tregs + TCVR_BBOENAB, 1);
> 
> You can remove one of the empty lines here.

OK

>> @@ -1196,15 +1182,15 @@ static void happy_meal_init_rings(struct happy_meal
>> *hp) struct hmeal_init_block *hb = hp->happy_block;
>>   	int i;
>>
>> -	HMD("happy_meal_init_rings: counters to zero, ");
>> +	HMD("counters to zero\n");
>>   	hp->rx_new = hp->rx_old = hp->tx_new = hp->tx_old = 0;
>>
>>   	/* Free any skippy bufs left around in the rings. */
>> -	HMD("clean, ");
>> +	HMD("clean\n");
> 
> I don't think this one is actually needed, there isn't much than can happen in
> between these 2 prints.

OK

>> @@ -1282,17 +1268,11 @@ happy_meal_begin_auto_negotiation(struct happy_meal
>> *hp, * XXX so I completely skip checking for it in the BMSR for now. */
>>
>> -#ifdef AUTO_SWITCH_DEBUG
>> -		ASD("%s: Advertising [ ");
>> -		if (hp->sw_advertise & ADVERTISE_10HALF)
>> -			ASD("10H ");
>> -		if (hp->sw_advertise & ADVERTISE_10FULL)
>> -			ASD("10F ");
>> -		if (hp->sw_advertise & ADVERTISE_100HALF)
>> -			ASD("100H ");
>> -		if (hp->sw_advertise & ADVERTISE_100FULL)
>> -			ASD("100F ");
>> -#endif
>> +		ASD("Advertising [ %s%s%s%s]\n",
>> +		    hp->sw_advertise & ADVERTISE_10HALF ? "10H " : "",
>> +		    hp->sw_advertise & ADVERTISE_10FULL ? "10F " : "",
>> +		    hp->sw_advertise & ADVERTISE_100HALF ? "100H " : "",
>> +		    hp->sw_advertise & ADVERTISE_100FULL ? "100F " :
> "");
>>
>>   		/* Enable Auto-Negotiation, this is usually on
> already... */
>>   		hp->sw_bmcr |= BMCR_ANENABLE;
> 
> Completely independent of this driver, but I wonder if there is no generic
> function to print these 10/100/* full/half duplex strings? There are several
> drivers doing this as a quick grep shows.

There's some functions to print just one link mode, but I think generally the
full advertising word is printed in debugs. I'm not too worried since this is
just for debug.

One of my goals is to convert this driver to phylib, but I haven't dived very
deep into it.

--Sean