Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <cf05390e-a69b-ad34-8c61-c5e9bbdaf5e3@gmail.com>
Date:   Mon, 19 Sep 2022 10:14:34 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH net-next 11/13] sunhme: Combine continued messages
Content-Language: en-US
To:     Rolf Eike Beer <eike-kernel@sf-tec.de>,
        "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org, Zheyu Ma <zheyuma97@gmail.com>,
        Nick Bowler <nbowler@draconx.ca>
References: <20220918232626.1601885-1-seanga2@gmail.com>
 <20220918232626.1601885-12-seanga2@gmail.com>
 <14992029.3CObj9AJNb@eto.sf-tec.de>
From:   Sean Anderson <seanga2@gmail.com>
In-Reply-To: <14992029.3CObj9AJNb@eto.sf-tec.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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