Re: [PATCH v2 1/1] staging/bcm: fix hostmibs.c checkpatch problems

From: Dan Carpenter
Date: Mon Mar 31 2014 - 04:47:14 EST

Next message: Greg Kurz: "Re: [PATCH] powerpc/le: enable RTAS events support"
Previous message: Takashi Iwai: "Re: [PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE"
In reply to: Jake Edge: "[PATCH v2 1/1] staging/bcm: fix hostmibs.c checkpatch problems"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Mar 28, 2014 at 12:49:08PM -0600, Jake Edge wrote:
>
> From: Jake Edge <jake@xxxxxxxxx>

Don't use this header when you're sending a patch you wrote yourself.
We prefer to get the information from the email header.

>
> Fix 4 checkpatch errors, many warnings in bcm/hostmibs.c
>
> Against next-20140328 tree

Put this below the --- cut off line.

>
> Signed-off-by: Jake Edge <jake@xxxxxxxxx>
> ---
>
> There were two > 80 lines that I couldn't find a sensible way to split
> up, otherwise it is checkpatch clean
>
> v2: fixes suggested by Joe Perches
>
> diff --git a/drivers/staging/bcm/hostmibs.c b/drivers/staging/bcm/hostmibs.c
> index 39ace55..d3b9adb 100644
> --- a/drivers/staging/bcm/hostmibs.c
> +++ b/drivers/staging/bcm/hostmibs.c
> @@ -9,37 +9,42 @@
>
> #include "headers.h"
>
> -INT ProcessGetHostMibs(struct bcm_mini_adapter *Adapter, struct bcm_host_stats_mibs *pstHostMibs)
> +INT ProcessGetHostMibs(struct bcm_mini_adapter *Adapter,
> + struct bcm_host_stats_mibs *pstHostMibs)
> {
> struct bcm_phs_entry *pstServiceFlowEntry = NULL;
> struct bcm_phs_rule *pstPhsRule = NULL;
> struct bcm_phs_classifier_table *pstClassifierTable = NULL;
> struct bcm_phs_classifier_entry *pstClassifierRule = NULL;
> - struct bcm_phs_extension *pDeviceExtension = (struct bcm_phs_extension *) &Adapter->stBCMPhsContext;
> + struct bcm_phs_extension *pDeviceExtension = &Adapter->stBCMPhsContext;
>

You may as well delete the blank line in the middle of the declaration
block while you are at it.

> - UINT nClassifierIndex = 0, nPhsTableIndex = 0, nSfIndex = 0, uiIndex = 0;
> + UINT nClassifierIndex = 0,
> + nPhsTableIndex = 0,
> + nSfIndex = 0,
> + uiIndex = 0;

There are now blanks at the end of the lines so it introduces new
checkpatch.pl warnings. Anyway, do it like this:

- UINT nClassifierIndex = 0, nPhsTableIndex = 0, nSfIndex = 0, uiIndex = 0;
+ UINT nClassifierIndex = 0;
+ UINT nPhsTableIndex = 0;
+ UINT nSfIndex = 0;
+ uiIndex = 0;

Really only "nPhsTableIndex" should be initialized here anyway, but
that's something for a different patch.

>
> if (pDeviceExtension == NULL) {
> - BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, HOST_MIBS, DBG_LVL_ALL, "Invalid Device Extension\n");
> + BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, HOST_MIBS,
> + DBG_LVL_ALL, "Invalid Device Extension\n");
> return STATUS_FAILURE;
> }
>
> /* Copy the classifier Table */
> - for (nClassifierIndex = 0; nClassifierIndex < MAX_CLASSIFIERS; nClassifierIndex++) {
> + for (nClassifierIndex = 0; nClassifierIndex < MAX_CLASSIFIERS;
> + nClassifierIndex++) {
> if (Adapter->astClassifierTable[nClassifierIndex].bUsed == TRUE)
> - memcpy((PVOID) &pstHostMibs->
> - astClassifierTable[nClassifierIndex],
> - (PVOID) &Adapter->
> - astClassifierTable[nClassifierIndex],
> + memcpy(&pstHostMibs->
> + astClassifierTable[nClassifierIndex],

I would prefer going over the 80 character limit rather than breaking
up the variable like this.

> + &Adapter->astClassifierTable[nClassifierIndex],
> sizeof(struct bcm_mibs_classifier_rule));
> }
>
> /* Copy the SF Table */
> for (nSfIndex = 0; nSfIndex < NO_OF_QUEUES; nSfIndex++) {
> if (Adapter->PackInfo[nSfIndex].bValid) {
> - memcpy((PVOID) &pstHostMibs->astSFtable[nSfIndex],
> - (PVOID) &Adapter->PackInfo[nSfIndex],
> - sizeof(struct bcm_mibs_table));
> + memcpy(&pstHostMibs->astSFtable[nSfIndex],
> + &Adapter->PackInfo[nSfIndex],
> + sizeof(struct bcm_mibs_table));
> } else {
> /* If index in not valid,
> * don't process this for the PHS table.
> @@ -70,7 +75,8 @@ INT ProcessGetHostMibs(struct bcm_mini_adapter *Adapter, struct bcm_host_stats_m
>
> memcpy(&pstHostMibs->
> astPhsRulesTable[nPhsTableIndex].u8PHSI,
> - &pstPhsRule->u8PHSI, sizeof(struct bcm_phs_rule));
> + &pstPhsRule->u8PHSI,
> + sizeof(struct bcm_phs_rule));
> nPhsTableIndex++;
>
> }
> @@ -82,53 +88,70 @@ INT ProcessGetHostMibs(struct bcm_mini_adapter *Adapter, struct bcm_host_stats_m
> /* Copy other Host Statistics parameters */
> pstHostMibs->stHostInfo.GoodTransmits = Adapter->dev->stats.tx_packets;
> pstHostMibs->stHostInfo.GoodReceives = Adapter->dev->stats.rx_packets;
> - pstHostMibs->stHostInfo.CurrNumFreeDesc = atomic_read(&Adapter->CurrNumFreeTxDesc);
> + pstHostMibs->stHostInfo.CurrNumFreeDesc =
> + atomic_read(&Adapter->CurrNumFreeTxDesc);
> pstHostMibs->stHostInfo.BEBucketSize = Adapter->BEBucketSize;
> pstHostMibs->stHostInfo.rtPSBucketSize = Adapter->rtPSBucketSize;
> pstHostMibs->stHostInfo.TimerActive = Adapter->TimerActive;
> pstHostMibs->stHostInfo.u32TotalDSD = Adapter->u32TotalDSD;
>
> - memcpy(pstHostMibs->stHostInfo.aTxPktSizeHist, Adapter->aTxPktSizeHist, sizeof(UINT32) * MIBS_MAX_HIST_ENTRIES);
> - memcpy(pstHostMibs->stHostInfo.aRxPktSizeHist, Adapter->aRxPktSizeHist, sizeof(UINT32) * MIBS_MAX_HIST_ENTRIES);
> + memcpy(pstHostMibs->stHostInfo.aTxPktSizeHist, Adapter->aTxPktSizeHist,
> + sizeof(UINT32) * MIBS_MAX_HIST_ENTRIES);
> + memcpy(pstHostMibs->stHostInfo.aRxPktSizeHist, Adapter->aRxPktSizeHist,
> + sizeof(UINT32) * MIBS_MAX_HIST_ENTRIES);

The prefered alignment for these is:

memcpy(pstHostMibs->stHostInfo.aRxPktSizeHist, Adapter->aRxPktSizeHist,
sizeof(UINT32) * MIBS_MAX_HIST_ENTRIES);

[tab][space][space][space][space][space][space][space]sizeof...

That way the 's' and the 'p' are in the same column.

>
> return STATUS_SUCCESS;
> }
>
> -VOID GetDroppedAppCntrlPktMibs(struct bcm_host_stats_mibs *pstHostMibs, struct bcm_tarang_data *pTarang)
> +VOID GetDroppedAppCntrlPktMibs(struct bcm_host_stats_mibs *pstHostMibs,
> + struct bcm_tarang_data *pTarang)
> {
> memcpy(&(pstHostMibs->stDroppedAppCntrlMsgs),
> &(pTarang->stDroppedAppCntrlMsgs),
> sizeof(struct bcm_mibs_dropped_cntrl_msg));
> }
>
> -VOID CopyMIBSExtendedSFParameters(struct bcm_mini_adapter *Adapter, struct bcm_connect_mgr_params *psfLocalSet, UINT uiSearchRuleIndex)
> +VOID CopyMIBSExtendedSFParameters(struct bcm_mini_adapter *Adapter,
> + struct bcm_connect_mgr_params *psfLocalSet, UINT uiSearchRuleIndex)

VOID CopyMIBSExtendedSFParameters(struct bcm_mini_adapter *Adapter,
struct bcm_connect_mgr_params *psfLocalSet,
UINT uiSearchRuleIndex)

[tab][tab][tab][tab][space][space]struct bcm_....

> {
> struct bcm_mibs_parameters *t = &Adapter->PackInfo[uiSearchRuleIndex].stMibsExtServiceFlowTable;
>
> t->wmanIfSfid = psfLocalSet->u32SFID;
> - t->wmanIfCmnCpsMaxSustainedRate = psfLocalSet->u32MaxSustainedTrafficRate;
> + t->wmanIfCmnCpsMaxSustainedRate =
> + psfLocalSet->u32MaxSustainedTrafficRate;
> t->wmanIfCmnCpsMaxTrafficBurst = psfLocalSet->u32MaxTrafficBurst;
> t->wmanIfCmnCpsMinReservedRate = psfLocalSet->u32MinReservedTrafficRate;
> t->wmanIfCmnCpsToleratedJitter = psfLocalSet->u32ToleratedJitter;
> t->wmanIfCmnCpsMaxLatency = psfLocalSet->u32MaximumLatency;
> - t->wmanIfCmnCpsFixedVsVariableSduInd = psfLocalSet->u8FixedLengthVSVariableLengthSDUIndicator;
> - t->wmanIfCmnCpsFixedVsVariableSduInd = ntohl(t->wmanIfCmnCpsFixedVsVariableSduInd);
> + t->wmanIfCmnCpsFixedVsVariableSduInd =
> + psfLocalSet->u8FixedLengthVSVariableLengthSDUIndicator;
> + t->wmanIfCmnCpsFixedVsVariableSduInd =
> + ntohl(t->wmanIfCmnCpsFixedVsVariableSduInd);
> t->wmanIfCmnCpsSduSize = psfLocalSet->u8SDUSize;
> t->wmanIfCmnCpsSduSize = ntohl(t->wmanIfCmnCpsSduSize);
> - t->wmanIfCmnCpsSfSchedulingType = psfLocalSet->u8ServiceFlowSchedulingType;
> - t->wmanIfCmnCpsSfSchedulingType = ntohl(t->wmanIfCmnCpsSfSchedulingType);
> + t->wmanIfCmnCpsSfSchedulingType =
> + psfLocalSet->u8ServiceFlowSchedulingType;
> + t->wmanIfCmnCpsSfSchedulingType =
> + ntohl(t->wmanIfCmnCpsSfSchedulingType);
> t->wmanIfCmnCpsArqEnable = psfLocalSet->u8ARQEnable;
> t->wmanIfCmnCpsArqEnable = ntohl(t->wmanIfCmnCpsArqEnable);
> t->wmanIfCmnCpsArqWindowSize = ntohs(psfLocalSet->u16ARQWindowSize);
> t->wmanIfCmnCpsArqWindowSize = ntohl(t->wmanIfCmnCpsArqWindowSize);
> - t->wmanIfCmnCpsArqBlockLifetime = ntohs(psfLocalSet->u16ARQBlockLifeTime);
> - t->wmanIfCmnCpsArqBlockLifetime = ntohl(t->wmanIfCmnCpsArqBlockLifetime);
> - t->wmanIfCmnCpsArqSyncLossTimeout = ntohs(psfLocalSet->u16ARQSyncLossTimeOut);
> - t->wmanIfCmnCpsArqSyncLossTimeout = ntohl(t->wmanIfCmnCpsArqSyncLossTimeout);
> + t->wmanIfCmnCpsArqBlockLifetime =
> + ntohs(psfLocalSet->u16ARQBlockLifeTime);
> + t->wmanIfCmnCpsArqBlockLifetime =
> + ntohl(t->wmanIfCmnCpsArqBlockLifetime);
> + t->wmanIfCmnCpsArqSyncLossTimeout =
> + ntohs(psfLocalSet->u16ARQSyncLossTimeOut);
> + t->wmanIfCmnCpsArqSyncLossTimeout =
> + ntohl(t->wmanIfCmnCpsArqSyncLossTimeout);
> t->wmanIfCmnCpsArqDeliverInOrder = psfLocalSet->u8ARQDeliverInOrder;
> - t->wmanIfCmnCpsArqDeliverInOrder = ntohl(t->wmanIfCmnCpsArqDeliverInOrder);
> - t->wmanIfCmnCpsArqRxPurgeTimeout = ntohs(psfLocalSet->u16ARQRxPurgeTimeOut);
> - t->wmanIfCmnCpsArqRxPurgeTimeout = ntohl(t->wmanIfCmnCpsArqRxPurgeTimeout);
> + t->wmanIfCmnCpsArqDeliverInOrder =
> + ntohl(t->wmanIfCmnCpsArqDeliverInOrder);
> + t->wmanIfCmnCpsArqRxPurgeTimeout =
> + ntohs(psfLocalSet->u16ARQRxPurgeTimeOut);
> + t->wmanIfCmnCpsArqRxPurgeTimeout =
> + ntohl(t->wmanIfCmnCpsArqRxPurgeTimeout);
> t->wmanIfCmnCpsArqBlockSize = ntohs(psfLocalSet->u16ARQBlockSize);
> t->wmanIfCmnCpsArqBlockSize = ntohl(t->wmanIfCmnCpsArqBlockSize);
> t->wmanIfCmnCpsReqTxPolicy = psfLocalSet->u8RequesttransmissionPolicy;

For this whole function, could we leave it as-is? The line breaks just
make it look messier. The right way to fix this is to rename the
struct members. What on earth does it add to put "wmanIfCmnCpsArq" at
the start of the name? (Nothing, it doesn't add anything, is what).

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Greg Kurz: "Re: [PATCH] powerpc/le: enable RTAS events support"
Previous message: Takashi Iwai: "Re: [PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE"
In reply to: Jake Edge: "[PATCH v2 1/1] staging/bcm: fix hostmibs.c checkpatch problems"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]