Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752326AbbBXPKR (ORCPT ); Tue, 24 Feb 2015 10:10:17 -0500 Received: from mail-lb0-f174.google.com ([209.85.217.174]:33256 "EHLO mail-lb0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751175AbbBXPKO (ORCPT ); Tue, 24 Feb 2015 10:10:14 -0500 MIME-Version: 1.0 In-Reply-To: <1424678898-3723-3-git-send-email-gbroner@codeaurora.org> References: <1424678898-3723-1-git-send-email-gbroner@codeaurora.org> <1424678898-3723-3-git-send-email-gbroner@codeaurora.org> Date: Wed, 25 Feb 2015 00:10:12 +0900 Message-ID: Subject: Re: [PATCH v3 2/4] scsi: ufs: add debugfs for ufs From: Akinobu Mita To: Gilad Broner Cc: Jej B , LKML , "linux-scsi@vger.kernel.org" , linux-arm-msm@vger.kernel.org, Santosh Y , linux-scsi-owner@vger.kernel.org, Subhash Jadavani , Yaniv Gardi , Dolev Raviv , Lee Susman , Raviv Shvili , Vinayak Holikatti , "James E.J. Bottomley" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4219 Lines: 99 2015-02-23 17:08 GMT+09:00 Gilad Broner : > From: Lee Susman > > Adding debugfs capability for ufshcd. > > debugfs attributes introduced in this patch: > - View driver/controller runtime data > - Command tag statistics for performance analisis > - Dump device descriptor info > - Track recoverable errors statistics during runtime > - Change UFS power mode during runtime > entry a string in the format 'GGLLMM' where: > G - selected gear > L - number of lanes > M - power mode > (1=fast mode, 2=slow mode, 4=fast-auto mode, > 5=slow-auto mode) > First letter is for RX, second is for TX. > - Get/set DME attributes I have a few nitpick comments on this patch. > +#ifdef CONFIG_DEBUG_FS > + > +#define UFSHCD_UPDATE_ERROR_STATS(hba, type) \ > + do { \ > + if (type < UFS_ERR_MAX) \ > + hba->ufs_stats.err_stats[type]++; \ > + } while (0) > + > +#define UFSHCD_UPDATE_TAG_STATS(hba, tag) \ > + do { \ > + struct request *rq = hba->lrb[task_tag].cmd ? \ > + hba->lrb[task_tag].cmd->request : NULL; \ > + u64 **tag_stats = hba->ufs_stats.tag_stats; \ > + int rq_type = -1; \ > + if (!hba->ufs_stats.enabled) \ > + break; \ > + tag_stats[tag][TS_TAG]++; \ > + if (!rq) \ > + break; \ > + WARN_ON(hba->ufs_stats.q_depth > hba->nutrs); \ > + if (rq_data_dir(rq) == READ) \ > + rq_type = TS_READ; \ > + else if (rq_data_dir(rq) == WRITE) \ > + rq_type = TS_WRITE; \ > + else if (rq->cmd_flags & REQ_FLUSH) \ > + rq_type = TS_FLUSH; \ > + else \ > + break; \ > + tag_stats[hba->ufs_stats.q_depth++][rq_type]++; \ > + } while (0) > + > +#define UFSHCD_UPDATE_TAG_STATS_COMPLETION(hba, cmd) \ > + do { \ > + struct request *rq = cmd ? cmd->request : NULL; \ > + if (cmd->request && \ > + ((rq_data_dir(rq) == READ) || \ > + (rq_data_dir(rq) == WRITE) || \ > + (rq->cmd_flags & REQ_FLUSH))) \ > + hba->ufs_stats.q_depth--; \ > + } while (0) > + > +#else > +#define UFSHCD_UPDATE_TAG_STATS(hba, tag) > +#define UFSHCD_UPDATE_TAG_STATS_COMPLETION(hba, cmd) > +#define UFSHCD_UPDATE_ERROR_STATS(hba, type) > + > +#endif Is there any reason that these are defined as macros instead of static functions? > @@ -5760,6 +5958,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq) > > async_schedule(ufshcd_async_scan, hba); > > + ufsdbg_add_debugfs(hba); > + > return 0; > > out_remove_scsi_host: > @@ -5769,6 +5969,7 @@ exit_gating: > out_disable: > hba->is_irq_enabled = false; > scsi_host_put(host); > + ufsdbg_remove_debugfs(hba); > ufshcd_hba_exit(hba); > out_error: > return err; This ufsdbg_remove_debugfs() call on error path of ufshcd_init() is unnecessary. Because ufsdbg_add_debugfs() is called at the last of ufshcd_init() and can't fail. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/