Hello Greg, Thank you for comments.
>-----Original Message----- >From: Greg KH <[email protected]> >Sent: Tuesday, December 11, 2018 5:34 PM >To: Parshuram Raju Thombare <[email protected]> >Cc: [email protected]; [email protected]; [email protected]; >[email protected]; [email protected]; >[email protected]; [email protected]; >[email protected]; [email protected]; [email protected]; >[email protected]; [email protected]; Alan Douglas ><[email protected]>; Janek Kotas <[email protected]>; Rafal Ciepiela ><[email protected]> >Subject: Re: [PATCH 2/2] scsi: ufs: add inline crypto support to UFS HCD > >EXTERNAL MAIL > > >On Tue, Dec 11, 2018 at 09:50:27AM +0000, Parshuram Thombare wrote: >> Add real time crypto support to UFS HCD using new device mapper >> 'crypto-ufs'. dmsetup tool can be used to enable real time / inline >> crypto support using device mapper 'crypt-ufs'. >> >> Signed-off-by: Parshuram Thombare <[email protected]> > >As you cc:ed me, I'll provide some minor review comments: > >> +config BLK_DEV_HW_RT_ENCRYPTION >> + bool >> + depends on SCSI_UFSHCD_RT_ENCRYPTION >> + default n > >n is always the default, you never need to list that. Ok, I will remove it. > >> + >> source block/Kconfig.iosched >> diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig index >> 2ddbb26..09a3ec0 100644 >> --- a/drivers/scsi/ufs/Kconfig >> +++ b/drivers/scsi/ufs/Kconfig >> @@ -136,3 +136,15 @@ config SCSI_UFS_BSG >> >> Select this if you need a bsg device node for your UFS controller. >> If unsure, say N. >> + >> +config SCSI_UFSHCD_RT_ENCRYPTION >> + bool "Universal Flash Storage Controller RT encryption support" >> + depends on SCSI_UFSHCD >> + default n > >Same here. > Ok, I will remove it. >> + select BLK_DEV_HW_RT_ENCRYPTION if SCSI_UFSHCD_RT_ENCRYPTION >> + select BLK_DEV_DM if SCSI_UFSHCD_RT_ENCRYPTION >> + help >> + Universal Flash Storage Controller RT encryption support >> + >> + Select this if you want to enable real time encryption on UFS controller >> + If unsure, say N. > >Don't you need to indent the help lines? > Sorry, missed indentation check here. I will indent this. >> +int ufshcd_crypto_init(struct ufs_hba *hba); void >> +ufshcd_crypto_remove(struct ufs_hba *hba); void >> +ufshcd_prepare_for_crypto(struct ufs_hba *hba, struct ufshcd_lrb >> +*lrbp); #endif > >You need to provide inline functions for when your config option is not enabled >here. > >That way you don't have to do this mess: > Do you mean empty inline function (which are exported) in #else to avoid #ifdef around each call for these functions ? >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index 86fe114..a96b038 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -47,6 +47,9 @@ >> #include "unipro.h" >> #include "ufs-sysfs.h" >> #include "ufs_bsg.h" >> +#ifdef CONFIG_SCSI_UFSHCD_RT_ENCRYPTION #include "ufshcd-crypto.h" >> +#endif > >No need for #ifdefs in .c files at all. Always include the .h file, and then >since your >functions are all inline void functions, the code just compiles away into >nothing. > > Ok, I will remove it. >> >> #define CREATE_TRACE_POINTS >> #include <trace/events/ufs.h> >> @@ -2198,6 +2201,14 @@ static void ufshcd_prepare_req_desc_hdr(struct >> ufshcd_lrb *lrbp, >> >> dword_0 = data_direction | (lrbp->command_type >> << UPIU_COMMAND_TYPE_OFFSET); >> +#ifdef CONFIG_SCSI_UFSHCD_RT_ENCRYPTION >> + if (lrbp->cci >= 0) { >> + dword_0 |= (1 << >CRYPTO_UTP_REQ_DESC_DWORD0_CE_SHIFT); >> + dword_0 |= ((lrbp->cci << >CRYPTO_UTP_REQ_DESC_DWORD0_CCI_SHIFT) >> + & >CRYPTO_UTP_REQ_DESC_DWORD0_CCI_MASK); >> + } else >> + dword_0 &= ~CRYPTO_UTP_REQ_DESC_DWORD0_CE_MASK; >> +#endif > >Some comments on what this is trying to do here? > > This is to enable encryption by setting CCI (crypto config index) and CE crypto enable bit. I will add comment here. >> if (lrbp->intr_cmd) >> dword_0 |= UTP_REQ_DESC_INT_CMD; >> >> @@ -2462,6 +2473,12 @@ static int ufshcd_queuecommand(struct Scsi_Host >*host, struct scsi_cmnd *cmd) >> lrbp->intr_cmd = !ufshcd_is_intr_aggr_allowed(hba) ? true : false; >> lrbp->req_abort_skip = false; >> >> +#ifdef CONFIG_SCSI_UFSHCD_RT_ENCRYPTION >> + lrbp->cci = -1; > >What does -1 mean? You should have a type for it if it is something "special". > -1 means crypto is not enabled or supported by hardware. I think this need a comment. I will add it. >> + /* prepare block for crypto */ >> + if (hba->capabilities & MASK_CRYPTO_SUPPORT) >> + ufshcd_prepare_for_crypto(hba, lrbp); #endif > >Again, no #ifdefs needed please. > > Ok, I will remove it. >> ufshcd_comp_scsi_upiu(hba, lrbp); >> >> err = ufshcd_map_sg(hba, lrbp); >> @@ -8119,6 +8136,11 @@ void ufshcd_remove(struct ufs_hba *hba) >> ufs_bsg_remove(hba); >> ufs_sysfs_remove_nodes(hba->dev); >> scsi_remove_host(hba->host); >> +#ifdef CONFIG_SCSI_UFSHCD_RT_ENCRYPTION >> + if (hba->capabilities & MASK_CRYPTO_SUPPORT) >> + ufshcd_crypto_remove(hba); >> +#endif > >Same ifdef here, and everywhere else in this file. > Ok, I will remove it. >> --- a/drivers/scsi/ufs/ufshcd.h >> +++ b/drivers/scsi/ufs/ufshcd.h >> @@ -193,6 +193,9 @@ struct ufshcd_lrb { >> ktime_t compl_time_stamp; >> >> bool req_abort_skip; >> +#ifdef CONFIG_SCSI_UFSHCD_RT_ENCRYPTION >> + int cci; >> +#endif > >No need for a #ifdef here. But comment on exactly what "cci" means, that seems >to not make any sense to me. > CCI is crypto config index / slot in UFS. I will add a comment here. >> }; >> >> /** >> @@ -706,6 +709,9 @@ struct ufs_hba { >> >> struct device bsg_dev; >> struct request_queue *bsg_queue; >> +#ifdef CONFIG_SCSI_UFSHCD_RT_ENCRYPTION >> + struct ufshcd_crypto_ctx *cctx; >> +#endif > >or here. > Ok, I will remove it. >> }; >> >> /* Returns true if clocks can be gated. Otherwise false */ diff --git >> a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h index >> 6fa889d..fe5a92d 100644 >> --- a/drivers/scsi/ufs/ufshci.h >> +++ b/drivers/scsi/ufs/ufshci.h >> @@ -90,6 +90,7 @@ enum { >> MASK_64_ADDRESSING_SUPPORT = 0x01000000, >> MASK_OUT_OF_ORDER_DATA_DELIVERY_SUPPORT = 0x02000000, >> MASK_UIC_DME_TEST_MODE_SUPPORT = 0x04000000, >> + MASK_CRYPTO_SUPPORT = 0x10000000, > >Why are you all not using the BIT(N) macro for these? Yes, I will replace it with BIT(N). > >thanks, > >greg k-h Regards, Parshuram Thombare
