| Message ID | 1442484140-13485-2-git-send-email-tony.cho@atmel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
On Thu, Sep 17, 2015 at 07:02:17PM +0900, Tony Cho wrote: > From: Leo Kim <leo.kim@atmel.com> > > This patch removes the potential faults which may happen when unexpectedly > getting access to invalid pointer. The pointer of pstrWFIDrv is unlikely > to be invalid. However, it is safer to return error when the invalid > memory is unfortunately accessed. > > Signed-off-by: Leo Kim <leo.kim@atmel.com> > Signed-off-by: Tony Cho <tony.cho@atmel.com> > --- > drivers/staging/wilc1000/host_interface.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c > index 6fdf392..151e8c4 100644 > --- a/drivers/staging/wilc1000/host_interface.c > +++ b/drivers/staging/wilc1000/host_interface.c > @@ -2403,8 +2403,10 @@ static s32 Handle_RcvdGnrlAsyncInfo(tstrWILC_WFIDrv *drvHandler, tstrRcvdGnrlAsy > s32 s32Err = 0; > tstrWILC_WFIDrv *pstrWFIDrv = (tstrWILC_WFIDrv *) drvHandler; > > - if (pstrWFIDrv == NULL) > + if (unlikely(!pstrWFIDrv)) { Can you measure the difference of using unlikely and not using it? If not, never use it, as odds are, the compiler and processor already guessed it correctly and made the code faster. If you can measure it, great, I'll be glad to take this patch, but you need to show your measurements in the changelog comments. > PRINT_ER("Driver handler is NULL\n"); > + return -EFAULT; -EFAULT is only for when we take a memory fault, which is not what is happening here. -ENODEV? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015? 09? 19? 11:50, Greg KH wrote: > On Thu, Sep 17, 2015 at 07:02:17PM +0900, Tony Cho wrote: >> From: Leo Kim <leo.kim@atmel.com> >> >> This patch removes the potential faults which may happen when unexpectedly >> getting access to invalid pointer. The pointer of pstrWFIDrv is unlikely >> to be invalid. However, it is safer to return error when the invalid >> memory is unfortunately accessed. >> >> Signed-off-by: Leo Kim <leo.kim@atmel.com> >> Signed-off-by: Tony Cho <tony.cho@atmel.com> >> --- >> drivers/staging/wilc1000/host_interface.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c >> index 6fdf392..151e8c4 100644 >> --- a/drivers/staging/wilc1000/host_interface.c >> +++ b/drivers/staging/wilc1000/host_interface.c >> @@ -2403,8 +2403,10 @@ static s32 Handle_RcvdGnrlAsyncInfo(tstrWILC_WFIDrv *drvHandler, tstrRcvdGnrlAsy >> s32 s32Err = 0; >> tstrWILC_WFIDrv *pstrWFIDrv = (tstrWILC_WFIDrv *) drvHandler; >> >> - if (pstrWFIDrv == NULL) >> + if (unlikely(!pstrWFIDrv)) { > Can you measure the difference of using unlikely and not using it? If > not, never use it, as odds are, the compiler and processor already > guessed it correctly and made the code faster. > > If you can measure it, great, I'll be glad to take this patch, but you > need to show your measurements in the changelog comments. I thought it twice and checked gcc documentation again. Finally, I was careless for that use. So, I will revert it and thank you for your advice. >> PRINT_ER("Driver handler is NULL\n"); >> + return -EFAULT; > -EFAULT is only for when we take a memory fault, which is not what is > happening here. -ENODEV? This will be replaced with a correct return value. Thank you.:-) > thanks, > > greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c index 6fdf392..151e8c4 100644 --- a/drivers/staging/wilc1000/host_interface.c +++ b/drivers/staging/wilc1000/host_interface.c @@ -2403,8 +2403,10 @@ static s32 Handle_RcvdGnrlAsyncInfo(tstrWILC_WFIDrv *drvHandler, tstrRcvdGnrlAsy s32 s32Err = 0; tstrWILC_WFIDrv *pstrWFIDrv = (tstrWILC_WFIDrv *) drvHandler; - if (pstrWFIDrv == NULL) + if (unlikely(!pstrWFIDrv)) { PRINT_ER("Driver handler is NULL\n"); + return -EFAULT; + } PRINT_D(GENERIC_DBG, "Current State = %d,Received state = %d\n", pstrWFIDrv->enuHostIFstate, pstrRcvdGnrlAsyncInfo->pu8Buffer[7]);