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;
Date:   Fri, 11 Feb 2022 17:25:28 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
Cc:     gregkh@linuxfoundation.org, realwakka@gmail.com,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: pi433: add rf69_dbg_hex function
Message-ID: <20220211142528.GO1951@kadam>
References: <YgYZRArwwF7Z1B4f@mail.google.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YgYZRArwwF7Z1B4f@mail.google.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?PASfzOc/kOj+0yLFbg8B6b2yjvb+1domI9zz4AoY4xMWPZtOhM+lN51yPA2m?=
 =?us-ascii?Q?Ppj441+LkziVppGTdR4vNG6f140b1yCtQ3rYRvDT6RxgWuObTXUdo4s5zXO3?=
 =?us-ascii?Q?skXZJTbmjJEy2JVSTZUMveSGxz8G+Ut+YOTIdf4h3YmIM7/bEwWbTET9koYh?=
 =?us-ascii?Q?u5fxPHpv0i9h5DfErgTJrhbyOSoHIZtXFdcZN2aeNb0yBedjTCZP2SroLVc6?=
 =?us-ascii?Q?X795BjSwUZizQ2Qtmbf0NSeBUl+yKeVyH2iKEadMYir6EeLpLKfLOGfOAwFb?=
 =?us-ascii?Q?JVDetKn94j5whzLeqN3ZFsmM1C5/k1RimoQkehgHN2Te8y3kMN8pBU6Mq7aq?=
 =?us-ascii?Q?zkAWXJSLDpckr6TlXUQz+mP1tCzRnuhce6KDB0wPOVpHdAqMk+VzsHsdm6WV?=
 =?us-ascii?Q?GHXDY6S0MlngsUMqJ9Fp8Cdi3XnWO7m8mYR5ZjZ5mDNVNRI02RVK5PokUHsM?=
 =?us-ascii?Q?eeGV7TD2QYzfzC16iettzqEc+4pzSsqfn8WsG2LkW8jFGwB2ARoMklb4oVBb?=
 =?us-ascii?Q?8Edb8LmctCjMXoEk3OWcRkPVI+r3jd8EOuL5cVKYVUH6w90htQppLKzBfoVc?=
 =?us-ascii?Q?xG+CnBwbnD9Ul+4JKS+04N44PV7gLWqk54LldLbEpU75C20WEDGX2XT2pGOQ?=
 =?us-ascii?Q?06aonCzGn8eTi+i7EAWnLF8Q2hN10Mxz/1mA5a1OdIiuF9JFsbl804KlJckY?=
 =?us-ascii?Q?i3B+baXSzu/YKfdmm0jehTrAz+8y+1QpCTTMCFAHBi0UYKT4dl1QB2sluQkw?=
 =?us-ascii?Q?jlTtFNXJU9hrfX2eKFJVFVQsJbnxuv8PTe92HEGscz957+oqSzyTRQXzf1aJ?=
 =?us-ascii?Q?C2h960AcKpI1QJNPNLR+tWLYhg2/Va9HgYFTlr1oVMBpT1fsicGhYV+OZQRF?=
 =?us-ascii?Q?YHI11fFoPb+TDm/Mt9WBPlsQ4jakZneV51ewjpU1Gtw7e6jF5VHcP1Zv2waE?=
 =?us-ascii?Q?yV21iVNL1I6KI9LEJEfjUpUHuLnmoQqSGVQLL8Cq8Vx/K1gAVxNEsYBLIaCT?=
 =?us-ascii?Q?NoqS36QL7qFHOeMrIlhwC0R2F/KiuMh8eFkw7GJcjvLNLXF45iBjRp99wxDP?=
 =?us-ascii?Q?E4CWVUNfdmUhz/pyn+t6ZMlh54DVR67gqRmCXMy6+XGkO8LFJE0pf+Q8M0M8?=
 =?us-ascii?Q?L4gqORNCPZLLn9xf2qFKujIS01KolWHompQWAOm8r/zfgdkys3ouOMBFQ3AG?=
 =?us-ascii?Q?VoSvaZkPHiM9lqr4gRNNioHY51B5866Hycdn7JOETS8HF8I5foA/BRlnCcHi?=
 =?us-ascii?Q?kPztmcy7DGxpb8ooSbNVG6j9um+ld6wp0ijN15PBgOh5YKrpx/ow3Qqk9k63?=
 =?us-ascii?Q?eUFBvKtQKQzAYD0YUhcHLgeOmD0hSWALh4HQuPPfU9MhC/FRMeAuBq8OnZ5n?=
 =?us-ascii?Q?wktUpaaT8Z0HJCnirw6jBKMrpU8/F75AnegYjQI90MpOY2DT+Ik5PXHJnmMb?=
 =?us-ascii?Q?FrDVCOBIU7V4ypme5ZPEDWoarjFOFdA7xf4NnD3WEmclYn1TLCokFtLrHkr4?=
 =?us-ascii?Q?BNys6hZWAWzlfiBvMY5THQ1GHPD5ovXgjEobLHz1t59ZWPx0wM9xghmaCzOE?=
 =?us-ascii?Q?9PRp3RuqHm8pMxeWwbkfcC+7jk/dXu9n2xfVBgtsZNiPSsTqa1oapk6hugt6?=
 =?us-ascii?Q?rbPQQJZQeN1gfInkXcz+fCFKVu2PHZ85uzVqf2n4wzOI6DlMRGBbOi5vD1rV?=
 =?us-ascii?Q?SCAWZw=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1f6bd96a-3525-4056-336f-08d9ed6a6083
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Feb 2022 14:25:39.5537
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: U2z+sQ4Kpe00ORtZfSh572+xoHIa/Q6Vx7lctirFInILq5HEJyUaE7dh3zO/dTAgXMnQDGL9vNdH1at644v2PC4W4PDy67gMBEBGgs7DP+I=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR10MB3249
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10254 signatures=673431
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 mlxscore=0 bulkscore=0
 malwarescore=0 suspectscore=0 phishscore=0 adultscore=0 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2201110000
 definitions=main-2202110079
X-Proofpoint-GUID: fFCh3UNPvycD004WIEt-WFTD8UmsoH8Z
X-Proofpoint-ORIG-GUID: fFCh3UNPvycD004WIEt-WFTD8UmsoH8Z
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Feb 11, 2022 at 09:07:32PM +1300, Paulo Miguel Almeida wrote:
> dev_<level> functions don't support printing hex dumps and the
> alternative available (print_hex_dump_debug) doesn't print the device
> information such as device's driver name and device name. That type of
> information which comes in handy for situations in which you can more
> than 1 device attached at the same type.
> 
> this patch adds a utility function that can obtain the same result as
> print_hex_dump_debug while being able to honour all possible flags that
> one may be interested in when dynamic debug is used.
> 
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> ---

I feel like this patch is over engineering your debug code.  Is this
really worthwhile?  If you really prefer the new format that's fine but
it seems like not necessarily a good use of energy.

> Meta-comments:
> 
> the initial discussion to use print_hex_dump_debug started in this patch
> but the original idea got merged into the brach.
> 
> https://lore.kernel.org/lkml/a630d8381cee0f543e0d77614052e1d04ab162a5.camel@perches.com/#t
> 
> ---
>  drivers/staging/pi433/rf69.c | 39 ++++++++++++++++++++++++++++--------
>  1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index 901f8db3e3ce..82d4ba24c35f 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -822,9 +822,37 @@ int rf69_set_dagc(struct spi_device *spi, enum dagc dagc)
>  
>  /*-------------------------------------------------------------------------*/
>  
> +static void rf69_dbg_hex(struct spi_device *spi, u8 *buf, unsigned int size,
> +			 const char *fmt, ...)
> +{
> +	va_list args;
> +	char textbuf[512] = {};

No need to initialize this.

> +	char *text = textbuf;
> +	int text_pos;
> +

Don't put a blank line in the middle of the declaration block.

> +	int rowsize = 16;
> +	int i, linelen, remaining = size;
> +
> +	va_start(args, fmt);
> +	text_pos = vscnprintf(text, sizeof(textbuf), fmt, args);
> +	text += text_pos;

When you're printing to a buffer like this then I find it's easier to
leave the start of the buffer constant.  So get rid of the text pointer
and just use "textbuf + prefix_len" instead of having a "text" pointer.


> +	va_end(args);
> +
> +	for (i = 0; i < size; i += rowsize) {
> +		linelen = min(remaining, rowsize);
> +		remaining -= rowsize;
> +
> +		hex_dump_to_buffer(buf + i, linelen, rowsize, 1,
> +				   text, sizeof(textbuf) - text_pos, false);
> +
> +		dev_dbg(&spi->dev, "%s\n", textbuf);
> +
> +		memset(text, 0, sizeof(textbuf) - text_pos);

No need for this.

> +	}

Instead of printing lineline at a time in a loop, what happens if you
just print size bytes?  (I honestly don't know because I have never used
this function before).

regards,
dan carpenter