| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20140331084650.GK6991@mwanda>
Date: Mon, 31 Mar 2014 11:46:50 +0300
From: Dan Carpenter <dan.carpenter@...cle.com>
To: Jake Edge <jake@...e2.net>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
devel@...verdev.osuosl.org, Joe Perches <joe@...ches.com>,
Jade Bilkey <herself@...fumon.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] staging/bcm: fix hostmibs.c checkpatch problems
On Fri, Mar 28, 2014 at 12:49:08PM -0600, Jake Edge wrote:
>
> From: Jake Edge <jake@...e2.net>
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@...e2.net>
> ---
>
> 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists