| 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: <96158616f0774e3d9fae0e99bb16e605@AUSX13MPS306.AMER.DELL.COM>
Date: Tue, 30 Oct 2018 21:26:57 +0000
From: <Justin.Lee1@...l.com>
To: <sam@...dozajonas.com>, <netdev@...r.kernel.org>
CC: <davem@...emloft.net>, <linux-kernel@...r.kernel.org>,
<openbmc@...ts.ozlabs.org>
Subject: RE: [PATCH net-next 5/6] net/ncsi: Reset channel state in
ncsi_start_dev()
> +int ncsi_reset_dev(struct ncsi_dev *nd)
> +{
> + struct ncsi_dev_priv *ndp = TO_NCSI_DEV_PRIV(nd);
> + struct ncsi_channel *nc, *active;
> + struct ncsi_package *np;
> + unsigned long flags;
> + bool enabled;
> + int state;
> +
> + active = NULL;
> + NCSI_FOR_EACH_PACKAGE(ndp, np) {
> + NCSI_FOR_EACH_CHANNEL(np, nc) {
> + spin_lock_irqsave(&nc->lock, flags);
> + enabled = nc->monitor.enabled;
> + state = nc->state;
> + spin_unlock_irqrestore(&nc->lock, flags);
> +
> + if (enabled)
> + ncsi_stop_channel_monitor(nc);
> + if (state == NCSI_CHANNEL_ACTIVE) {
> + active = nc;
> + break;
Is the original intention to process the channel one by one?
If it is the case, there are two loops and we might need to use
"goto found" instead.
> + }
> + }
> + }
> +
found: ?
> + if (!active) {
> + /* Done */
> + spin_lock_irqsave(&ndp->lock, flags);
> + ndp->flags &= ~NCSI_DEV_RESET;
> + spin_unlock_irqrestore(&ndp->lock, flags);
> + return ncsi_choose_active_channel(ndp);
> + }
> +
> + spin_lock_irqsave(&ndp->lock, flags);
> + ndp->flags |= NCSI_DEV_RESET;
> + ndp->active_channel = active;
> + ndp->active_package = active->package;
> + spin_unlock_irqrestore(&ndp->lock, flags);
> +
> + nd->state = ncsi_dev_state_suspend;
> + schedule_work(&ndp->work);
> + return 0;
> +}
Also similar issue in ncsi_choose_active_channel() function below.
> @@ -916,32 +1045,49 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
>
> ncm = &nc->modes[NCSI_MODE_LINK];
> if (ncm->data[2] & 0x1) {
> - spin_unlock_irqrestore(&nc->lock, flags);
> found = nc;
> - goto out;
> + with_link = true;
> }
>
> - spin_unlock_irqrestore(&nc->lock, flags);
> + /* If multi_channel is enabled configure all valid
> + * channels whether or not they currently have link
> + * so they will have AENs enabled.
> + */
> + if (with_link || np->multi_channel) {
I notice that there is a case that we will misconfigure the interface.
For example below, multi-channel is not enable for package 1.
But we enable the channel for ncsi2 below (package 1 channel 0) as that interface is the first
channel for that package with link.
cat /sys/kernel/debug/ncsi_protocol/ncsi_device_
IFIDX IFNAME NAME PID CID RX TX MP MC WP WC PC CS PS LS RU CR NQ HA
=====================================================================
2 eth2 ncsi0 000 000 1 1 1 1 1 1 0 2 1 1 1 1 0 1
2 eth2 ncsi1 000 001 1 0 1 1 1 1 0 2 1 1 1 1 0 1
2 eth2 ncsi2 001 000 1 0 1 0 1 1 0 2 1 1 1 1 0 1
2 eth2 ncsi3 001 001 0 0 1 0 1 1 0 1 0 1 1 1 0 1
=====================================================================
MP: Multi-mode Package WP: Whitelist Package
MC: Multi-mode Channel WC: Whitelist Channel
PC: Primary Channel CS: Channel State IA/A/IV 1/2/3
PS: Poll Status LS: Link Status
RU: Running CR: Carrier OK
NQ: Queue Stopped HA: Hardware Arbitration
I temporally change to the following to avoid that.
if ((with_link &&
!np->multi_channel &&
list_empty(&ndp->channel_queue)) || np->multi_channel) {
> + spin_lock_irqsave(&ndp->lock, flags);
> + list_add_tail_rcu(&nc->link,
> + &ndp->channel_queue);
> + spin_unlock_irqrestore(&ndp->lock, flags);
> +
> + netdev_dbg(ndp->ndev.dev,
> + "NCSI: Channel %u added to queue (link %s)\n",
> + nc->id,
> + ncm->data[2] & 0x1 ? "up" : "down");
> + }
> +
> + spin_unlock_irqrestore(&nc->lock, cflags);
> +
> + if (with_link && !np->multi_channel)
> + break;
Similar issue here. As we are using break, so each package will configure one active TX.
> }
> + if (with_link && !ndp->multi_package)
> + break;
> }
>
> - if (!found) {
> + if (list_empty(&ndp->channel_queue) && found) {
> + netdev_info(ndp->ndev.dev,
> + "NCSI: No channel with link found, configuring channel %u\n",
> + found->id);
> + spin_lock_irqsave(&ndp->lock, flags);
> + list_add_tail_rcu(&found->link, &ndp->channel_queue);
> + spin_unlock_irqrestore(&ndp->lock, flags);
> + } else if (!found) {
> netdev_warn(ndp->ndev.dev,
> - "NCSI: No channel found with link\n");
> + "NCSI: No channel found to configure!\n");
> ncsi_report_link(ndp, true);
> return -ENODEV;
> }
Also, for deselect package handler function, do we want to set to inactive here?
If we just change the state, the cached data still keeps the old value. If the new
ncsi_reset_dev() function is handling one by one, can we skip this part?
static int ncsi_rsp_handler_dp(struct ncsi_request *nr)
{
struct ncsi_rsp_pkt *rsp;
struct ncsi_dev_priv *ndp = nr->ndp;
struct ncsi_package *np;
struct ncsi_channel *nc;
unsigned long flags;
/* Find the package */
rsp = (struct ncsi_rsp_pkt *)skb_network_header(nr->rsp);
ncsi_find_package_and_channel(ndp, rsp->rsp.common.channel,
&np, NULL);
if (!np)
return -ENODEV;
/* Change state of all channels attached to the package */
NCSI_FOR_EACH_CHANNEL(np, nc) {
spin_lock_irqsave(&nc->lock, flags);
nc->state = NCSI_CHANNEL_INACTIVE;
spin_unlock_irqrestore(&nc->lock, flags);
}
return 0;
}
Powered by blists - more mailing lists